Flake8 Round 2: We have to go deeper #158
Conversation
Now all public functions need a docstring @hrshdhgd I'll leave this for you!
sssom/context.py
Outdated
| @@ -51,6 +73,11 @@ def add_built_in_prefixes_to_prefix_map( | |||
|
|
|||
|
|
|||
| def get_default_metadata() -> Metadata: | |||
| """Get Default metadata. | |||
There was a problem hiding this comment.
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.
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.
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.
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.
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.
tests/test_data.py
Outdated
| """Get all test cases. | ||
|
|
||
| :return: List of test cases | ||
| :rtype: List[SSOMTestCase] |
There was a problem hiding this comment.
use type annotations so this is checked with mypy (also there's a typo)
|
@cthoyt Danke danke wie immer!.. :) Do we have to fix all your comments to be able to pass the PR? I would like to move to a smaller PR model - not because I don't like big changes (I do), but because we need to address some functional issues now, and these code-quality PRs do conflict a lot with these. Can I trust that:
Thanks for advice! |
@hrshdhgd removing CI checks is never the answer, especially in the case of black where the fix is so easy
Just keep in mind it's not just about passing CI, but also about doing documentation and style well. The cascade of type checking is actually a feature - if many things rely on each other and become out of sync, it's good that mypy fails. I can't give any general guidance other than that when this happens, you should really do some digging to figure out where the type annotations went wrong - read all of the other functions that seemed to break |
This function was 100% redundant of the pandas read_csv function, which already supports skipping lines starting with a comment character, reading from URLs, reading from filepaths, and reading from textios. All of the typing was just causing issues after this
It's also reflected in the type annotation. Not sure why it got added in.
|
Okay guys, thanks so far for the monumental effort on this PR. Everything is passing CI, and I've gone through and made tweaks to everything that had a comment on it (importantly including reverting that change I made on |
As a follow-up to #156, this one enables even more flake8 checks from the
flake8-docstringsplugin. I did a few of the tiny ones, but now the meatiest part is to addressD103- all public functions need a docstring. I'm going to conscript @hrshdhgd for this bitKeep in mind all of this flake8/mypy/code style stuff is a bit of a pain in the short term, but it will keep us sane as developers in the future and support the longevity of a community that might come to rely on this package at some point