-
Notifications
You must be signed in to change notification settings - Fork 382
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
Add types hints for ingredients and experiment #543
Conversation
@@ -36,6 +39,8 @@ | |||
|
|||
PYTHON_IDENTIFIER = re.compile("^[a-zA-Z_][_a-zA-Z0-9]*$") | |||
|
|||
PathType = Union[str, bytes, Path] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we use os.PathLike
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't actually find a clear answer to this question. I though about looking into the python stub file for os.path but it's not actually informative. https://github.com/python/typeshed/blob/edee7cfb3cc5af16afa557f1279c22ee6663a61f/stdlib/3/os/path.pyi#L14 . I'm open to any idea.
From simple experiments, it seems that
isinstance(pathlib.Path('.'), os.PathLike)
returns true but
isinstance('dodo', os.PathLike)
returns False.
Any guidance is welcome.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True. Also this discussion python/typing#402 seems to state that there is the idea to have a protocol class for this at some time, but currently it is something different. I would then probably use
PathType = Union[str, bytes, os.PathLike]
That's also what this section in the original PEP states https://www.python.org/dev/peps/pep-0519/#provide-specific-type-hinting-support. The alternative would be to use only PathLike
and force the user to wrap their pathlib.Path
though I do not really see so much benefit in it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we slowly migrate the internals of sacred to pathlib? That would make the code easier to read. We can do so while staying backward compatible of course.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
os.pathlike is not available for python 3.5. That solves the issue. Going back to pathlib.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, migrating sacred internals to pathlib sounds like a very good idea!
Great idea! |
This reverts commit 20162ac.
Awesome, thanks! Adding type-annotations and eventually mypy checking to the tests has been on my list too. I'm very happy that you got started on that already! And +1 for |
Adding type hints on the API surface allows IDEs and static code analysis tools to do a better job at detecting user errors.
It's also quite informative when using IDEs as you can get hints in the auto-completion.