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.

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

Clarify that temporal and bbox args must be tuples in docstrings, fix docstring formatting #448

Merged
merged 16 commits into from
Feb 8, 2024

Conversation

mfisher87
Copy link
Member

@mfisher87 mfisher87 commented Feb 6, 2024

Addresses #279, but does not resolve it. This work exposed and addresses a couple of concerns, but there are some still I think that need more work.

Docstring formatting

On the main branch, there are some formatting errors that are causing key information to be omitted from the docs. It seems really easy to make these mistakes, and Mkdocs doesn't complain about them! How can we protect ourselves?

  • We could really use some linting rules that can catch docstrings that are not formatted properly in the Google style. Ruff has some docstring PEP linters, but I don't think that's enough here. Can Ruff lint for Google docstring style?
  • We need to be very careful with line-continuation. Line continuation must be done with an indent or everything on the next line will be either omitted or misformatted. A better option, IMO, is to use block style like many of the changes in this PR.
  • We need to be very careful with whitespace. A blank line under Parameters: will cause the entire parameters block to be formatted like a code block instead of parsed.
  • We have redundant type information in docstrings, e.g. strategy (String): duplicates the type (str) in the function signature. Mkdocs will pick up the type from the function signature so we don't need to keep these values in sync over time.

📚 Documentation preview 📚: https://earthaccess--448.org.readthedocs.build/en/448/

@mfisher87 mfisher87 changed the title Clarify that temporal and bbox args must be tuples in docstrings Clarify that temporal and bbox args must be tuples in docstrings, fix API docstring formatting Feb 6, 2024
- pip
- pip:
- poetry
- markdown-callouts>=0.2.0
Copy link
Member Author

Choose a reason for hiding this comment

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

This enabled me to build docs without messing with Poetry. Wasn't sure if this is intentional, as CONTRIBUTING.md doesn't really specify.

earthaccess/results.py Outdated Show resolved Hide resolved
earthaccess/results.py Outdated Show resolved Hide resolved
earthaccess/results.py Outdated Show resolved Hide resolved
andypbarrett
andypbarrett previously approved these changes Feb 7, 2024
Copy link
Collaborator

@andypbarrett andypbarrett left a comment

Choose a reason for hiding this comment

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

In my opinion, this clarifies the inputs.

@mfisher87 mfisher87 changed the title Clarify that temporal and bbox args must be tuples in docstrings, fix API docstring formatting Clarify that temporal and bbox args must be tuples in docstrings, fix docstring formatting Feb 7, 2024
Copy link
Collaborator

@danielfromearth danielfromearth left a comment

Choose a reason for hiding this comment

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

I believe this is ready to go now.

@mfisher87
Copy link
Member Author

Wow, thanks Daniel for going all out on these docstrings!!! We needed this :)

@danielfromearth
Copy link
Collaborator

danielfromearth commented Feb 8, 2024

@mfisher87 , @jhkennedy , @andypbarrett , we could enable linting of docstrings via ruff this way. What do you think?

I just ran it and got many more things to potentially fix...

snapshot

@mfisher87
Copy link
Member Author

mfisher87 commented Feb 8, 2024

Heck yes, thank you for looking in to that!!! Should we break that in to another PR so we can merge this one?

@danielfromearth
Copy link
Collaborator

danielfromearth commented Feb 8, 2024

Heck yes, thank you for looking in to that!!! Should we break that in to another PR so we can merge this one?

sounds good to me 👍

Once any one else approves it —e.g., @mfisher87, @andypbarrett? — I'll merge it.

Copy link
Collaborator

@jhkennedy jhkennedy left a comment

Choose a reason for hiding this comment

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

This looks good to me!

earthaccess/api.py Show resolved Hide resolved
@danielfromearth danielfromearth merged commit 5805b14 into main Feb 8, 2024
13 checks passed
@danielfromearth danielfromearth deleted the issue-279-clarify-docs branch February 8, 2024 22:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants