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.
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
Flake8 Round 2: We have to go deeper #158
Flake8 Round 2: We have to go deeper #158
Changes from 14 commits
12d352f
fd719e8
d1bd442
5248308
8c3c25e
15fbe19
bd504d9
23048b0
405e7b0
2d0baf6
d9998fa
4833f18
484ed3f
c183999
4fa46e5
b47274d
0eba665
b060b28
68c838f
a46642e
e2d5235
f532dcc
8574a54
6fd4230
b16e887
a1b83cf
0854096
1e7cf51
d298e67
260dfbf
88a606d
3673552
8315f05
abee8cb
2673fc3
d0d9b34
02184f9
fdc5c97
0f576bb
0e9f681
9295e6f
e1f1a6f
b4a7bd8
294a246
8c02a4a
329ff49
cf80b47
cf612a4
f4991f6
0f74c64
6a2cce8
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.
src should be Optional[str] -> see the None check in the definition of the 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.
obviously it defaults to none, that's the default. what happens if None is passed?
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.
@matentzn
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's almost never sufficient to just re-write the name of the function. What is default metadata? How was it chosen? Imagine a user could read this docstring and not know what it is or where
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.
Don't be afraid to be redundant of other function's docstrings, since documentation readers might only find the one function they want to know more about.
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.
Agreed. For this PR to pass to I need to fix all these? Or should fixing these just be on the longer agenda?
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.
Yes. The purpose of this PR is cleanup, so if the job isn't done (and also done well) then there's no advantage to merging it. As you can see, it's indeed possible to game flake8 by writing unhelpful docstrings. It's not really helpful for users to read documentation that just barely passes flake8 - we have to go the extra step and make sure that the docstrings can educate and inform users. As it's the case that we as the developers and maintainers don't currently know what some functions do... well this is not a great situation to be in. It means we need to put in the legwork to figure out what all of the code does (even if we weren't the ones who wrote it).
One alternative is to scale back the scope of this PR (i.e., reinstate the ignore for D103) and delete the stub docstrings, but I think that it's rather the case that @hrshdhgd just needs a bit of training and practice on writing useful docstrings. Tbh after a quick google search, I didn't find any material that really showed what a good docstring was other than the minimum syntactical and semantic requirements for it to be readable by Sphinx, so this will take some effort and a bit more back-and-forth in the comments.
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.
Another note on why this should be done well - it's 99% likely that once the D103 check (this is the one that makes sure functions have docstrings with the correct style) that many of them will never be touched again. If we allow an unhelpful docstring to pass through review, then it will never be fixed, and we'll never have a flake8 error to check it for us, and never another pass flake8 campaign where we organize effort to fix this.
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.
how does it split the dataframe? why?
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.
@matentzn
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.
which object?
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.
not helpful
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.
what does it mean to contract a URI
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's not a URI string if it's been contracted. Then it's a prefix.