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.

Sign up for GitHub

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

Jump to bottom

Apply pre-commit on docstrings - fix invalid Python in docs #3220

Merged
merged 4 commits into from
May 18, 2021
Merged

Apply pre-commit on docstrings - fix invalid Python in docs #3220

merged 4 commits into from
May 18, 2021

Conversation

cphyc
Copy link
Member

@cphyc cphyc commented Apr 21, 2021

Attempts to use https://github.com/cphyc/pre-commit-all-the-way-down/ on our docstrings.

@cphyc
Copy link
Member Author

cphyc commented Apr 21, 2021

I think https://github.com/cphyc/pre-commit-all-the-way-down/ is too slow at the moment to be used in prod. There are many ways it can improve in the future (and I intend to do it), but in the meantime I think this PR can go as is!

@cphyc cphyc marked this pull request as ready for review April 21, 2021 19:22
@cphyc cphyc changed the title Update pydoc Apply pre-commit on docstrings Apr 21, 2021
neutrinoceros
neutrinoceros approved these changes Apr 21, 2021
View reviewed changes
Copy link
Member

@neutrinoceros neutrinoceros left a comment

Choose a reason for hiding this comment

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

Thanks a lot for doing this ! Beyond the consistency and arguable "prettiness" improvements here, the mere fact that you were able to run black there means that you validated the doctests as valid Python. It doesn't mean they pass, but that's certainly an excellent first step to perform before we can make them part of CI via pytest.

@neutrinoceros neutrinoceros added the infrastructure Related to CI, versioning, websites, organizational issues, etc label Apr 21, 2021
@neutrinoceros
Copy link
Member

Classifying this as Infra (CI) by anticipation.

@neutrinoceros neutrinoceros added the code style Related to linting tools label Apr 21, 2021
@cphyc cphyc changed the title Apply pre-commit on docstrings Apply pre-commit on docstrings - fix invalid Python in docs Apr 29, 2021
@cphyc cphyc added the bug label Apr 29, 2021
@cphyc
Apply pre-commit on docstrings
bedba16 
This is obtained by using pre-commit-all-the-way-down (https://github.com/cphyc/pre-commit-all-the-way-down) on the codebase, applying only isort, pyupgrade and black
@cphyc
Copy link
Member Author

cphyc commented Apr 30, 2021 •
edited
Loading

I have tagged it as "bug" because it fixes several invalid code snippets in our docstrings (see e.g. https://github.com/yt-project/yt/pull/3220/files#diff-459eb66cb29de05519a40f30db1bf3653d4c13c115829dd48cc4409fc3149e0bR2344, where the final ) was missing).
This PR only checks that the syntax is valid, not that it makes sense or that variables are properly defined.

@matthewturk
Copy link
Member

LGTM. Got some conflicts, but I think it should go in once those are resolved.

@neutrinoceros neutrinoceros merged commit ddcb73d into yt-project:main May 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@neutrinoceros neutrinoceros neutrinoceros approved these changes

@matthewturk matthewturk matthewturk approved these changes

Assignees
No one assigned
Labels
bug code style Related to linting tools infrastructure Related to CI, versioning, websites, organizational issues, etc
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@cphyc @neutrinoceros @matthewturk