-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
[review wanted] add support for locally installed typehints #1895
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
Conversation
I like this idea, but I think it needs some more comments explaining what's happening. I also suspect that the paths where to look are actually platform-dependent (I doubt that on Windows looking in |
How do you plan to address these questions? |
I've just added some comments and a system independant way to find the data dir. |
CI is failling because of missing site module in typeshed, here it goes typeshed#389. |
@JukkaL: Can you have a look at this? This seems an innocuous PR but it establishes a new place where type hints might be installed. @tharvik: I think this needs an update to the docs with examples of how a package can install its own stubs. (And you should test all this?) Also, maybe default_data_dir() could benefit from some of the constants used here (site.USER_BASE, sys.prefix)? |
I like the idea. Should we look for stubs under both I agree with @gvanrossum in that the locations should be described in the docs. I'd rather have complete paths instead of talking about
If you can't test some of the environments we can try to find somebody who can help. |
@gvanrossum I'll update the docs then, having something similar as PEP 484; I don't have much time this week, I'll try to find a bit of time but I'm not such. |
@tharvik It's probably okay to use |
e939f46
to
f181130
Compare
bf61a5e
to
96e4d45
Compare
Here goes the updated documentation and the updated default paths, it was tested on Linux system python and Linux virtualenv python. |
96e4d45
to
b7f6b9e
Compare
Mypy will also look in some default install directory to try to find annotations. | ||
For example, for UNIX based, with python 3.4, it will try to find it in | ||
- ``${HOME}/.local/shared/typehints/python3.4/`` | ||
- ``/usr/shared/typehints/python3.4/`` |
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.
The standard directory is /usr/share/
(no d
). And I think it would be better to put python*
before typehints
in the path, to mirror /usr/lib/pythonX.Y/
. I'd also rather use /usr/lib/python*
for this rather than introduce a second python tree under /usr/share
(and maybe put it under site-packages
?).
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.
It is actually based on the example in PEP 484 in chapter store and distribute stub files on how to add it to a setup.py.
...
data_files=[
(
'shared/typehints/python{}.{}'.format(*sys.version_info[:2]),
pathlib.Path(SRC_PATH).glob('**/*.pyi'),
),
],
...
The standard directory is /usr/share/ (no d).
From the example, it is indeed 'shared', the '/usr' is because setup.py will install data_files
under sys.prefix
on system-wide install (based on install info on docs.python.org)
And I think it would be better to put python* before typehints in the path, to mirror /usr/lib/pythonX.Y/.
That's taken from the example, so that would require a PEP change, but I guess it can easily be introduce as it isn't really implemented for now.
I'd also rather use /usr/lib/python* for this rather than introduce a second python tree under /usr/share (and maybe put it under site-packages?).
/usr/lib/python* is for modules, not really for related data; IMO both options are valid, because typehints are closely related to code, but in itself, it isn't necessary for the code to run, thus not going into site-packages. /usr/ is also the standard place to store modules' related data.
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'm referring to the filesystem hierarchy standard: http://www.pathname.com/fhs/pub/fhs-2.3.html#USRSHAREARCHITECTUREINDEPENDENTDATA
If the PEP says /usr/shared
, then the PEP should be corrected before this part of it is implemented.
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.
Hum, I see, and that makes sense, it's nicer. Then that would be a design choice coming from @gvanrossum or @JukkaL, and a separated issue IMO.
But I disagreed on 'before this part of it is implemented', we can merge it for now, and update mypy when the path changes are accepted in the PEP.
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.
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'll try to rephrase/intepret what was said, correct me if I'm wrong
- not having a separated python tree outside of
/usr/lib/python*/site-packages
- at least, using UNIX standard install location (not
/usr/shared
but/usr/share
for example)
I guess, a way to do it would be to change the PEP to not use data_files
but rather use package_data
, as these are closer to the implementation than having a separated tree.
@bdarnell is that okay for you?
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.
package_data
is fine with me.
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've opened peps#67 about it, we can move the discussion there.
Hey @tharvik, It looks like this PR is pretty much stuck. I'm trying to clean up stuck PRs so that the list of PRs is not too long. If you feel like working on this some more, just go ahead and reopen it, or open a new PR -- the corresponding issue will remain open with a reference to this (closed) PR in it, so if someone else feels like tackling this problem they still have this PR as a starting point. |
Implement example given in PEP 484, in chapter on how to store and distribute stub files, this is functional but not really nice, would be better to implement it via
pkg_resources
at module loading time.Prefer locally installed stubs (
--user
) rather than system wide for easier user customization. Also, in general, one of the most important stubs location, more than typeshed, as it might be more up to date than it.