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

expand importlib #183

Closed
wants to merge 1 commit into from
Closed

expand importlib #183

wants to merge 1 commit into from

Conversation

tharvik
Copy link
Contributor

@tharvik tharvik commented May 5, 2016

improve importlib.

based on stubgen mainly.

@tharvik tharvik force-pushed the extend_importlib branch from 8f3350c to da86f3c Compare May 5, 2016 11:57
@gvanrossum
Copy link
Member

@brettcannon Do you think it makes sense to add all these submodules and internal classes? How much of this is considered a stable/documented API? I think stuff that's not part of the stable API should be left out of the stubs -- it's just asking for problems when the internals get updated in a future version but the stub is behind.

@brettcannon
Copy link
Member

brettcannon commented May 6, 2016

Don't include most of that. :) The vast majority of it is private and even that which isn't is exposed through other modules as part of the public API (e.g. ModuleSpec is exposed through importlib.machinery, not through importlib._bootstrap which is an implementation detail).

If you want stubs for importlib then feel free to open an issue for it and assign it to me and I will hand-write appropriate type hints.

@gvanrossum
Copy link
Member

OK, created #189, closing this one.

@gvanrossum gvanrossum closed this May 6, 2016
@tharvik
Copy link
Contributor Author

tharvik commented May 9, 2016

I think stuff that's not part of the stable API should be left out of the stubs -- it's just asking for problems when the internals get updated in a future version but the stub is behind.

ok, I'll try to avoid submitting too much internal.
btw, I was getting ready to document the io module, but it does import _io (in the current stubs), which is internal right? so, what to do? do not typecheck _io (but then, having some not documented stubs is not really useful, and mypy in parano mode will still warn about it)? move needed definition from _io to io (but then it will expose some unwanted classes)? still typecheck _io, hoping it won't change much (but then, how to know which are changing, which module are "worth" to be documented)?

@gvanrossum
Copy link
Member

gvanrossum commented May 9, 2016 via email

@brettcannon
Copy link
Member

So should the type stubs match where the code is defined (e.g. _io), or where it is publicly exposed (e.g. io)?

@gvanrossum
Copy link
Member

I'm not 100% sure I understand your question. :-(

Answering the most likely interpretation, in cases where we decide not to completely hide the private _xxx version, the stubs for it should reveal the truth but only for those things that are used or re-exported by the public xxx version. In cases where we use the convention that xxx essentially reimplements xxx (e.g. pickle) we probably should not expose _xxx at all. But I don't think that's the case for _io -- the io.py module doesn't work if the _io extension can't be imported.

@brettcannon
Copy link
Member

OK, concrete example: importlib.machinery.ModuleSpec is actually implemented in importlib._bootstrap. That's entirely a technical detail to clearly separate out code that has to be written without using any import statement due to bootstrapping requirements and in no way is meant to be exposed to the user.

So my question is do you want the type hints written for ModuleSpec as it it lived in importilb.machinery, importlib._bootstrap, or both?

@gvanrossum
Copy link
Member

In that case it sounds fine to leave _bootstrap out of it entirely.

@tharvik tharvik deleted the extend_importlib branch May 10, 2016 12:58
momandine pushed a commit to momandine/typeshed that referenced this pull request Jul 5, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants