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

Some docstring cleanup #590

Closed
danielfromearth opened this issue Jun 5, 2024 · 3 comments · Fixed by #596
Closed

Some docstring cleanup #590

danielfromearth opened this issue Jun 5, 2024 · 3 comments · Fixed by #596
Labels
impact: documentation Improvements or additions to documentation

Comments

@danielfromearth
Copy link
Collaborator

danielfromearth commented Jun 5, 2024

A couple of additional docstring suggestions pulled from this comment in PR #580:

@danielfromearth danielfromearth added the impact: documentation Improvements or additions to documentation label Jun 5, 2024
@mfisher87
Copy link
Collaborator

Thanks @danielfromearth !

@Sherwin-14
Copy link
Contributor

@danielfromearth Hey Daniel, I had some doubts regarding this issue. First of all, on removing the type annotations means we'll have to remove all the instances in the code related to isn't that the case? I just wanted some clarity over this. Also the kerchunk.py has two public functions and in the issue description you had suggested that we'll need to add the "Placeholder" to only one public function in there. So which one is it? It will help a lot if you chime in.

@mfisher87
Copy link
Collaborator

mfisher87 commented Jun 9, 2024

I think what we're looking for is on this line:

https://github.com/Sherwin-14/earthaccess/blob/f8901ade489c8c211658ec13a5757ce1b6259ff9/earthaccess/auth.py#L96

there's a docstring "type annotation" indicating the type of system is Env. In Python code without real type annotations, this can be useful, but since we have real type annotations, it's redundant. It's also incorrect, the real type of system is Optional[System]. So we're just looking to remove the docstring type annotation.

Personally, I think we should not add Placeholder anywhere else. We used that solely to enable docstrings with metadata (Parameters or Returns) but without a subject line to not be borked by the code formatter. We considered adding a subject line out of scope for that PR.

Sherwin-14 added a commit to Sherwin-14/earthaccess that referenced this issue Jun 11, 2024
Sherwin-14 added a commit to Sherwin-14/earthaccess that referenced this issue Jun 11, 2024
Sherwin-14 added a commit to Sherwin-14/earthaccess that referenced this issue Jun 11, 2024
Sherwin-14 added a commit to Sherwin-14/earthaccess that referenced this issue Jun 11, 2024
Sherwin-14 added a commit to Sherwin-14/earthaccess that referenced this issue Jun 11, 2024
mfisher87 added a commit that referenced this issue Jun 12, 2024
Did the necessary docstring cleanup as discussed in #590
@github-project-automation github-project-automation bot moved this from 🆕 New to ✅ Done in earthaccess project Jun 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
impact: documentation Improvements or additions to documentation
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

3 participants