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

Type annotation for retrieve's known_hash should be str or None #332

Closed
BjoernLudwigPTB opened this issue Jan 16, 2023 · 4 comments · Fixed by #333
Closed

Type annotation for retrieve's known_hash should be str or None #332

BjoernLudwigPTB opened this issue Jan 16, 2023 · 4 comments · Fixed by #333
Labels
documentation Improvements or additions to documentation

Comments

@BjoernLudwigPTB
Copy link
Contributor

BjoernLudwigPTB commented Jan 16, 2023

Page that contains the error or needs to be improved:

https://www.fatiando.org/pooch/latest/api/generated/pooch.retrieve.html#pooch.retrieve

Description of the problem/suggestion:

The page states as allowed type only str but None can be provided as well, as the documentation reflects the code behaviour correctly. None is not of type str, so my IDE (and everybody else's as well I suppose) complains about None being provided. I know it is not recommended, but still it is implemented and expected to be possible. The documentation should reflect that. I am happy to provide a quick PR on that, if that is desired. I assume the only change needed would be in line 79 like:

    known_hash : str or None
@BjoernLudwigPTB BjoernLudwigPTB added the documentation Improvements or additions to documentation label Jan 16, 2023
@santisoler
Copy link
Member

Hi @BjoernLudwigPTB! Thanks for opening this issue. You are absolutely right, the known_hash parameter could be a str or None and the docstring should be clear about that. Would you like to open a PR to fix it?

BTW, which IDE are you using? Do you know what code parser or language server is the one that complains about it? Does it parse the docstring of the function to check for valid types? I'm just asking out of curiosity.

@BjoernLudwigPTB
Copy link
Contributor Author

BjoernLudwigPTB commented Jan 17, 2023

I am using PyCharm and I guess they are parsing the docstring as well. Unfortunately I don't know any details on how they conclude but usually they seem pretty well informed. 😉 Pooch's code base does not contain any type hints yet, right? Are you guys planing on introducing them?

@santisoler
Copy link
Member

Thanks for the info on your IDE, it's good to know what other people are using and how they interact with our code.

We don't have type hints on any Fatiando library. And we are not still sure if we are going to introduce them, I think we are still considering the pros and cons, and the work it requires to implement them in entire libraries. I think you should follow #330 if you're interested in this.

Thanks again for your contribution @BjoernLudwigPTB!

@BjoernLudwigPTB
Copy link
Contributor Author

Ah, nice, thanks for the heads up! I will browse through your extensive answer there. I am glad I could help out here!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants