Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add pre-commit hooks for linting #2177

Merged
merged 18 commits into from
Jul 12, 2022
Merged

Add pre-commit hooks for linting #2177

merged 18 commits into from
Jul 12, 2022

Conversation

gsheni
Copy link
Contributor

@gsheni gsheni commented Jul 11, 2022

  • This makes it so that linting is automatically run right before you commit code:
  • We use this https://pre-commit.com/
  • Example of this:
    Screen Shot 2022-07-11 at 5 30 10 PM

Gaurav Sheni added 2 commits July 11, 2022 16:40
@gsheni gsheni self-assigned this Jul 11, 2022
@gsheni gsheni marked this pull request as ready for review July 11, 2022 21:30
@gsheni
Copy link
Contributor Author

gsheni commented Jul 11, 2022

@sbadithe Can checkout this branch and make sure the steps in contributing.md work for you? They are as follows:

pip install pre-commit
pre-commit install
pre-commit run

@sbadithe
Copy link
Contributor

@sbadithe Can checkout this branch and make sure the steps in contributing.md work for you? They are as follows:

pip install pre-commit
pre-commit install
pre-commit run

Commands run for me, but it skips all the checks as it says there are no files to check

@gsheni
Copy link
Contributor Author

gsheni commented Jul 11, 2022

@sbadithe Yes, it will only run on the changed files

sbadithe
sbadithe previously approved these changes Jul 11, 2022
contributing.md Outdated
* Before you commit, a few lint fixing hooks will run. You can also manually run these.
```bash
pip install pre-commit
pre-commit install
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to list the pip install pre-commit and pre-commit install commands or will that be covered when users run make installdeps?

I created a fresh environment and just ran make installdeps and it seems like everything worked.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm...upon further review, you might actually have to run pre-commit install before the hooks actually run on a commit on a fresh clone of the repo.

Maybe we also need to add pre-commit install to the make installdeps command?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed:
de40da2

Copy link
Contributor

@thehomebrewnerd thehomebrewnerd left a comment

Choose a reason for hiding this comment

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

LGTM

@gsheni gsheni merged commit 73744e7 into main Jul 12, 2022
@gsheni gsheni deleted the add_pre_hooks branch July 12, 2022 14:46
This was referenced Jul 19, 2022
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants