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 checks #22
Conversation
keirthana
commented
Jul 18, 2023
- Add a pre-commit hook for inclusive naming check
- Fix issues reported by the pre-commit hook
README.rst
Outdated
3. Navigate to the top-level directory of this repository and run ``pre-commit install --install-hooks``. | ||
4. The following checks are run automatically before every commit: | ||
- Inclusive naming checks (``woke``). | ||
5. To ignore any violation reported by ``woke``, see `<https://docs.getwoke.tech/ignore/>`_. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The woke documentation is a bit vague in places. E.g. (1), it says you can ignore files by adding to your config file - but which file specifically? E.g. (2) Their in-line ignoring refers to a "following line" and then doesn't have a following line!
Some brief instructions here in our readme would be helpful, I think. Just the method for "ignore a whole file" and "ignore a line" should do it. If we have a preferred method for inline/nextline, we should pick one and present/stick to that to make sure that our docs are mutually consistent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand, the intention behind linking to the documentation was so that we don't have outdated instructions in case the woke decides to change something. I will add some preliminary ignoring instructions and then maybe redirect for more information.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good to me! Thanks :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks for adding this. Will let Dominik review/merge to make sure I haven't overlooked anything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Praise: Nice addition, LGTM
Note: I will merge this once my 2 comments are addressed.
d37ecdd
to
eb8a27c
Compare
eb8a27c
to
af69180
Compare
This is technically not needed as the `.pre-commit-config.yaml` file specifies that woke only checks `.rst` files, but this may avoid unwanted warnings/checks in the future if the config should chnage in the future.