Skip to content

Conversation

salilmishra23
Copy link
Contributor

For issue #132, added the following hooks:

  • check-yaml
  • trailing-whitespace
  • requirements-txt-fixer
  • end-of-file-fixer
  • black

We can add tests and mypy too as hooks but can keep that for another PR.

Let me know if you want to add/remove hooks.

@lalitpagaria lalitpagaria self-requested a review July 4, 2021 13:34
@lalitpagaria
Copy link
Collaborator

@salilmishra23 thank you very much for your PR.

I will review it by tomorrow and share feedback.

@lalitpagaria lalitpagaria requested a review from GirishPatel July 5, 2021 03:56
Copy link
Collaborator

@lalitpagaria lalitpagaria left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@salilmishra23 Thank you for working on it. I have reviewed your PR. It is looking nice I have few comments. Can you please address them

Also can you please add information about how to perform pre-commit check in contribution guideline

@salilmishra23
Copy link
Contributor Author

salilmishra23 commented Jul 7, 2021

@lalitpagaria I have added the mypy hook for pre-commit.

Does adding pytest as a hook seem feasible at this point of time? It has to be run after every commit which takes time specially due to downloading of the huge model. It might not be helpful for people making small quick changes.

Anyways let me know what you think about this. I have no other problem if you want me to add pytest hook.

@lalitpagaria
Copy link
Collaborator

@salilmishra23 You PR is complete functionality wise. Can you please add information about how to perform pre-commit check in contribution guideline. Post it I will approve and merge this PR.

Okay let's skip pytest for now, we can later add when we feel a need for pytest in pre-commit.

Does adding pytest as a hook seem feasible at this point of time? It has to be run after every commit which takes time specially due to downloading of the huge model. It might not be helpful for people making small quick changes.

@lalitpagaria lalitpagaria removed the request for review from GirishPatel July 7, 2021 19:03
Copy link
Collaborator

@lalitpagaria lalitpagaria left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!
@salilmishra23 Thank you for working on this.

@lalitpagaria lalitpagaria merged commit 99379da into obsei:master Jul 7, 2021
@lalitpagaria lalitpagaria mentioned this pull request Jul 7, 2021
@salilmishra23 salilmishra23 deleted the pre-commit-integration branch July 7, 2021 19:08
@lalitpagaria lalitpagaria linked an issue Jul 8, 2021 that may be closed by this pull request
@lalitpagaria lalitpagaria added the enhancement New feature or request label Oct 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add pre-commit hook
2 participants