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 pylint docstrings to precommit hook #113

Closed
Michael-T-McCann opened this issue Nov 22, 2021 · 6 comments · Fixed by #141 or #145
Closed

Add pylint docstrings to precommit hook #113

Michael-T-McCann opened this issue Nov 22, 2021 · 6 comments · Fixed by #141 or #145
Labels
developer Developer environment: issues related to CI, git, etc.

Comments

@Michael-T-McCann
Copy link
Contributor

We current check for docstrings on PRs, but it would be nice to have this checked client-side as part of the precommit hook. This would also involve changing the dev requirements and updating the docs.

@bwohlberg bwohlberg added the developer Developer environment: issues related to CI, git, etc. label Nov 23, 2021
@SauravMaheshkar
Copy link
Contributor

@bwohlberg Can I take this up ? I'd love to contribute to the repository and this seems like a nice "good first issue".

I'll make changes to the .pre-commit-config.yaml, the dev requirements and the docs.

@bwohlberg
Copy link
Collaborator

Thanks @SauravMaheshkar, that would be great.

@SauravMaheshkar
Copy link
Contributor

Request for clarification: @bwohlberg In which section should I add pylint ? In the Tests section or the Contributing Code section ?

@bwohlberg
Copy link
Collaborator

The only existing reference I can find to the pre-commit hook is in point 9. of Installing a Development Version, so the minimal change necessary for consistency would seem to simply involve adding a mention of pylint to that paragraph.

@Michael-T-McCann: Any additional thoughts on this?

@Michael-T-McCann
Copy link
Contributor Author

Unfortunately, the solution in #141 seems to run pylint on every file in the commit, including files that are normally skipped when running pylist scico. In particular, if your commit touches a test file, you now must add docstrings to all the tests before you can commit.

@bwohlberg
Copy link
Collaborator

Note also the --score=n added to .pre-commit-config.yaml in #143.

@bwohlberg bwohlberg linked a pull request Dec 22, 2021 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
developer Developer environment: issues related to CI, git, etc.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants