fatiando/pooch

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

BjoernLudwigPTB opened this issue · 4 comments

BjoernLudwigPTB commented

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
santisoler commented

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 commented

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 commented

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!

  • 🚀1
BjoernLudwigPTB commented

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

  • 👍1