Added linting of docstrings #580
Conversation
|
@mfisher87 Hey, the hooks failed on this one because it had picked up the errors identified by ruff. How can we tackle this? |
There was a problem hiding this comment.
Wow, that produced a lot of new linting errors 😆 😅
I think we can, for now, ignore the D1* errors in the pyoroject.toml settings because it's going to be a lot of work to add docstrings to every module, class, and function.
Ruff seems to have autofixers for most of the D2* and D4* errors. I'd try running ruff with ruff check --fix, and if that doesn't fix enough things, try ruff check --fix --unsafe-fixes. If you need to use unsafe fixes, please keep an extra eye out for anything weird :)
|
@Sherwin-14 I'd highly recommend using Ruff with pre-commit instead of installing it standalone. Once you have installed pre-commit, you can use it to install hooks for this project with We also use it on the Benefit Tool project. |
|
@mfisher87 Hey Matt, even with your suggestion of unsafe fixes we still have 258 unsolved errors. |
|
Sounds like it auto-fixed about 500 errors, nice! Can you commit and push those? Then I can see what kind of errors are left. Maybe we want to set those to ignored. What about ignoring the |
|
I found 159 errors barring D1* |
|
Can you push those changes up? |
|
Surprised |
|
Woops, didn't mean to make this draft. |
|
I think I should push the changes, adding D417 errors to the ignore list as well. What do you think about this? Also fixing some of those issues is something I too considered ;) |
Sherwin-14
force-pushed
the
linting
branch
3 times, most recently
from
dc87dd7 to
caad446
Compare
There was a problem hiding this comment.
LGTM, thanks @Sherwin-14 !
Will plan to merge tomorrow just in case anyone wants to take a last look. CC participants of the discussion that originated this need: @danielfromearth @jhkennedy @andypbarrett
|
Yay, I'm glad to see these changes! One thing that I don't mind one way or the other, but I'm curious... Docstrings like this (in auth.py): def get_s3_credentials(
self,
daac: Optional[str] = None,
provider: Optional[str] = None,
endpoint: Optional[str] = None,
) -> Dict[str, str]:
"""Gets AWS S3 credentials for a given NASA cloud provider.
The easier way is to use the DAAC short name; provider is optional if we know it.
Parameters
----------
daac: The name of a NASA DAAC, e.g. NSIDC or PODAAC.
provider: A valid cloud provider. Each DAAC has a provider code for their cloud distributions.
endpoint: Getting the credentials directly from the S3Credentials URL.
Returns
-------
A Python dictionary with the temporary AWS S3 credentials.
"""...it looks to me like a combination of Google style (with each parameter indented) and numpy style (using |
|
Good catch. It actually looks like the docs aren't rendering correctly. Should have checked that. I think we need a tweak :) |
There was a problem hiding this comment.
@Sherwin-14 I forgot a critical piece! If you check the links in the post above this one, the docs aren't rendering correctly anymore because I forgot we need to set the correct style.
[tool.ruff.lint.pydocstyle]
convention = "google"
To re-apply the rules, we need to run the --fix (maybe also unsafe) operation. We may need/want to un-do the previous fix operation (keeping only changes to pyproject.toml) prior to re-running the fix.
|
Ruff does not specify a convention by default, select however can be used to specify a small subset of a convention so you can mix and match different parts of a convention which I think is what is going on. |
True, though we use the google convention, so we can use the
EDIT: I think I mis-read your post and I'm just re-stating what you said 😆 |
|
Docs are looking great now! 🥳 |
There was a problem hiding this comment.
Hey Sherwin, sorry for another wrench in the works! Looks like we're so close to being there but Ruff seems to have caused some doc pages to render incorrectly because it was too aggressive with trying to populate a subject line. We'll need to do a bit of manual editing to fix it as far as I can tell. If you feel comfortable writing a docstring subject line, that would be so appreciated! But otherwise we can just leave it as TODO. or Placeholder. so the docs render correctly and come back to it later.
If those manual edits pan out to be too time consuming, feel free to ask for help :)
earthaccess/search.py
Outdated
| """???+ Info | ||
| The DataCollection class queries against | ||
| https://cmr.earthdata.nasa.gov/search/collections.umm_json, | ||
| the response has to be in umm_json to use the result classes. |
There was a problem hiding this comment.
Looks like the linter messed up the indentation on this info box , so now it renders as empty (here: https://earthaccess--580.org.readthedocs.build/en/580/user-reference/collections/collections-query/)! :(
I think this happened because there's no subject line in the docstring like there should be. I think we need to add that. A placeholder would be OK, since there's nothing there right now. Are you OK with adding Placeholder. as the first line for docstrings missing a subject? (NOTE: No need to add any new docstrings)
earthaccess/results.py
Outdated
| Returns: | ||
| The data visualization links, usually the browse images. | ||
| """Returns: | ||
| The data visualization links, usually the browse images. |
There was a problem hiding this comment.
The docs for this function also didn't render correctly because of a missing subject line :(
|
Hey Matt, wouldn't it be better to configure ruff to ignore this!
…On Sun, 26 May, 2024, 9:57 pm Matt Fisher, ***@***.***> wrote:
***@***.**** commented on this pull request.
Hey Sherwin, sorry for another wrench in the works! Looks like we're *so
close* to being there but Ruff seems to have caused some doc pages to
render incorrectly because it was too aggressive with trying to populate a
subject line. We'll need to do a bit of manual editing to fix it as far as
I can tell. If you feel comfortable writing a docstring subject line, that
would be so appreciated! But otherwise we can just leave it as TODO. or
Placeholder. so the docs render correctly and come back to it later.
If those manual edits pan out to be too time consuming, feel free to ask
for help :)
------------------------------
In earthaccess/search.py
<#580 (comment)>:
> @@ -79,11 +77,10 @@ def get_results(
class DataCollections(CollectionQuery):
- """
- ???+ Info
- The DataCollection class queries against
- https://cmr.earthdata.nasa.gov/search/collections.umm_json,
- the response has to be in umm_json to use the result classes.
+ """???+ Info
+ The DataCollection class queries against
+ https://cmr.earthdata.nasa.gov/search/collections.umm_json,
+ the response has to be in umm_json to use the result classes.
Looks like the linter messed up the indentation on this info box , so now
it renders as empty (here:
https://earthaccess--580.org.readthedocs.build/en/580/user-reference/collections/collections-query/)!
:(
I think this happened because there's no subject line in the docstring
like there should be. I think we need to add that. A placeholder would be
OK, since there's nothing there right now. Are you OK with adding
Placeholder. as the first line for docstrings missing one?
------------------------------
In earthaccess/results.py
<#580 (comment)>:
> @@ -325,9 +314,8 @@ def data_links(
return https_links
def dataviz_links(self) -> List[str]:
- """
- Returns:
- The data visualization links, usually the browse images.
+ """Returns:
+ The data visualization links, usually the browse images.
The docs for this function also didn't render correctly because of a
missing subject line :(
https://earthaccess--580.org.readthedocs.build/en/580/user-reference/granules/granules/#earthaccess.results.DataGranule.dataviz_links
—
Reply to this email directly, view it on GitHub
<#580 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/BBV63P7FHOYEF344LBWNJ6LZEIEOLAVCNFSM6AAAAABIBQAZ4KVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDANZZGY4DOMZZHE>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
What are you suggesting we configure ruff to ignore? If we can turn off a rule and fix that behavior I'm for it :) |
…ist and added placeholders.
|
@mfisher87 Hey Matt, apparently numpy is an implicit dependency brought by one of your packages. This version however, requires distutils which has been removed from Python 3.12 . |
We don't yet officially support Python 3.12, but thanks for the reminder. I have an unfinished PR that I have to wrap up. Part of the remaining work in that PR is specifically to address this issue by distinguishing numpy versions based upon Python versions. See #571. |
Co-authored-by: Matt Fisher <[email protected]>
There was a problem hiding this comment.
Nice! @chuckwondo @danielfromearth any last minute thoughts?
earthaccess/results.py
Outdated
| The collection data type, i.e. HDF5, CSV etc., if available. | ||
| """Placeholder. | ||
|
|
||
| Returns: The collection data type, i.e. HDF5, CSV etc., if available. |
There was a problem hiding this comment.
This change is actually breaking some documentation:
The return description is no longer being parsed correctly when this change is applied.
|
@mfisher87 Hey sorry about that I have fixed the issue now.you can take a look at it ;) |
There was a problem hiding this comment.
This looks good! A couple of additional docstring suggestions:
- I think this comment for
rebuild_auth()can be put into the method docstring instead - there's an out-dated and unnecessary type annotation here that can be removed
- I think Google docstring style prefers a spacing between summary line and expanded description (see examples here). Here are the places I found that look like a line spacing could be added:
- in refresh_tokens()
- in get_s3_credentials()
- in the two hits methods: hits() and hits()
- in provider()
- in set_requests_session()
- in get_fsspec_session()
- in get_requests_session()
- since we're adding "Placeholder" for the subject line in docstrings in other places, perhaps we should also add them for:
- the two public functions in
daac.py - the one public function in
kerchunk.py - make_instance()
- the two public functions in
(I wish I could've just used the "suggest changes" feature in GitHub, but it seems like for now you can only do that on lines that have already been changed)
|
Hey @danielfromearth these are all great ideas! I'd like to keep the scope on this PR as small as possible, though. We only added "Placeholder" where we needed it to enable the other parts of the docstring to work without the new linter rules messing stuff up, for example where there was no subject line but there were parameter definitions. Would you mind creating an issue for those items? |
There was a problem hiding this comment.
Things look great, docs look great... last call anyone? @danielfromearth @chuckwondo ?
|
All good by me. |
|
🚀 @Sherwin-14 Thanks so much for your help with locking down our docstrings! |
I have added the linting for docstrings. Please have a look at it ;) Resolves #454
📚 Documentation preview 📚: https://earthaccess--580.org.readthedocs.build/en/580/