Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
ENH: Use flake8 to check for PEP8 violations in doctests #23399
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
ENH: Use flake8 to check for PEP8 violations in doctests #23399
Changes from 7 commits
c87360d
bc009a4
0d730ce
cb477f8
762de5d
0358c21
9bc7f65
6090b24
72f99bd
affd8f4
561f3c3
384c77f
1178cae
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 think
doctest
is not a very descriptive name if this test is to check whether pep8 should fail because of unknownnp
. The rest of the example is very verbose to just check that. I'd use the description to explain what is this test about. And in the examples just with>>> np.nan
could be enough.Then, couple more tests could be added, to test the typical pep8 issues we have, like
>>> foo = 1+2
(missing spaces)...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 reason to use flake8 to check for pep8 issues was to not have to write all these tests. flake8 should be tested enough to work properly.
This test basically only verifies that flake8 is actually testing the docstrings.
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 didn't realize before, but there is something tricky here. The convention is to assume numpy and pandas are always imported before the examples. I personally would be explicit, but that's how all the examples are made.
This means two things:
math
?)And as I said in the previous review, I'd like to have more examples of pep8 failures, like missing spaces around an operator. Also, these examples are too long for what is being tested.
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 think it complicates things unnecessarily importing objects other than modules.
import contextlib
andimport flake8.main.application
seems like the simplest and best way. And then use accordinly@contextlib.contextmanager
andapp = flake8.main.application.Application
. This way, just by reading those lines it's obvious what is the module and what is being used, and there is not need to go to the imports and start decoding aliases.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'd personally prefer to
yield from application.guide.stats.statistics_for('')
instead of returning the string. In case the information is used in more than one place, having the components can be useful.Also, it's an opinion, but I wouldn't use a property here, as this feels more like an action than an attribute. A method
validate_pep8
seems easier to understand.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.
This name is not very descriptive. I think this is the examples as temporary file, so something like
_examples_as_temp_file
seems much clearer.Not sure if it's worth having two separate methods, I'd generate the file in the method that validates pep8. But if we have two methods, I'd have this first, as this is being used by the other (
pep8_violations
).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.
we use numpydoc, not this syntax
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.
As said, I have a preference to have only the examples code in the temporary file. Extracting the code from the examples is trivial, and once #23161 is merged we'll have a property in this class that returns the lines of code.
What you will have to do is before writing the code in the examples, write
import numpy as np
and same for pandas, so flake8 does not complain because of that.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'm a bit confused with this. I guess I'm missing something. If in our docstring we have something like:
I'd expect to have in the temporary file to validate:
As we won't validate the description, or anything else. I think numpydoc returns the lines with code, so writing to the file should be a single line of code. Is there any advantage generating a function?
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.
Well flake8 supports doctests by adding
--doctests
as an argument. And the Examples follow the doctest standard.Having to carve out the examples is not trivial and would require writing multiple tests. So creating a file with a dummy-function is a simple hack, getting ugly cause it has to restore the indentation of the docstring. One test of the
BadExamples
class ensures it is working.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.
it'd be better to not do unrelated changes