Add mypy to CI job and type hints for one class. #404
Conversation
|
💖 Thank you for opening your first pull request in this repository! 💖 A few things to keep in mind:
⭐ No matter what, we are really grateful that you put in the effort to do this! ⭐ |
|
Hi @santisoler, is there anything I could do to help move this along? This change would be helpful to me when working with this library, and it seems like there has been interest from others as well. |
There was a problem hiding this comment.
Hi @eliotwrobson. Yes, I totally agree. Type hints is something people would like to see in Pooch (me included!).
I left a few comments here so we can move forward with this PR.
Importing from typing
I noticed that in this PR you import typing as t, and then use each one of the types in signatures with t.<TYPE>, like t.Optional, t.List`, etc.
Why not importing the required types directly? Like from typing import .... Most of the codebases that use type hints use the modules like this:
from typing import Optional
def f(x: Optional[float] = None):
...I think having that extra t. creates some clutter in the signatures, specially when we need to use several types. What do you think?
Do we need typing_extensions?
I noticed that you added typing_extensions as a dependency. I'm curious if this strictly necessary. I left a comment below where we can continue the conversation.
Minor things to do
Before I forget, we need to add the new style requirements to environment.yml as well. Thanks for adding them to requirements-style.txt and to add a new target to the Makefile!
Thanks again for the effort you put in this! I really appreciate that we can move forward with type hints in Pooch!
| types-tqdm | ||
| types-paramiko | ||
| mypy | ||
| pytest |
There was a problem hiding this comment.
Is there a reason why we need pytests for checking styles?
There was a problem hiding this comment.
Mypy is run over the test case files as well, so this needs pytest installed to recognize the import.
pooch/core.py
Outdated
| FilePath = t.Union[str, os.PathLike] | ||
| Actions = te.Literal["download", "fetch", "update"] | ||
|
|
||
|
|
||
| class DownloaderT(te.Protocol): | ||
| """ | ||
| A class used to define the type definition for the downloader function. | ||
| """ | ||
|
|
||
| # pylint: disable=too-few-public-methods | ||
| def __call__( # noqa: E704 | ||
| self, | ||
| fname: str, | ||
| action: t.Optional[FilePath], | ||
| pooch: "Pooch", | ||
| *, | ||
| check_only: t.Optional[bool] = None, | ||
| ) -> t.Any: ... | ||
|
|
||
|
|
||
| ProcessorT = t.Callable[[str, Actions, "Pooch"], t.Any] |
There was a problem hiding this comment.
I think I'd like to separate the public classes from the type variables in different submodules. What about defining a pooch.typing submodule where these can live?
On a side note, would it be feasible to name them without the T? Or will it create more confusion?
There was a problem hiding this comment.
I think that would work. The T is my convention but I can remove that as well.
EDIT: It seems like separating this would cause a circular import because there's a method that takes in a callable that calls Pooch. This also seems to cause runtime errors for some reason.
|
@santisoler I've responded to your comments. I've added mypy to the environment.yml, removed typing_extensions, and changed the typing import. Also, let me know if you want any help with #453, since I'm currently using ruff to autoformat everything locally. |
Changes to test type hints and integrating a typechecker into the CI job.
This was to test feasibility of adding type annotations to the package (per
the issue). This was surprisingly easy to do, I basically just copied the types
listed in the docstring.
Relevant issues/PRs:
#330