Skip to content

Change prefix for _importlib_modulespec names #1127

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Change prefix for _importlib_modulespec names #1127

wants to merge 1 commit into from

Conversation

pkch
Copy link
Contributor

@pkch pkch commented Apr 2, 2017 •
edited
Loading

(Not ready for merging)

This PR is meaningless without python/mypy#3107; if that one is approved, this would fix the type reported for modules from _importlib_modulespec.ModuleType to types.ModuleType. (There is no dependency in the other direction.)

This PR is related to python/mypy#3106.

@pkch pkch force-pushed the importlib-modulespec-names branch from 72b69c9 to c63f855 Compare April 2, 2017 05:14
@pkch
Copy link
Contributor Author

pkch commented Apr 2, 2017

Next commit would break mypy unless python/mypy#3107 is merged.

Note: I wasn't sure about this line in 2/abc.pyi:

types = ...  # type: module

which I changed to

types = ...  # type: ModuleType

I'm afraid I might be breaking something even though the tests pass.

Also, werkzeug stubs had:

class module(ModuleType):
  def __getattr__(self, name): ...
  def __dir__(self): ...

I removed them, but I also suspect they may serve some non-obvious purpose.

JelleZijlstra
JelleZijlstra reviewed Apr 2, 2017
View reviewed changes
stdlib/2/tokenize.pyi Outdated
@@ -102,15 +103,15 @@ chain = ... # type: type
double3prog = ... # type: type
endprogs = ... # type: Dict[str, Any]
pseudoprog = ... # type: type
re = ... # type: module
re = ... # type: ModuleType
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This probably shouldn't be here at all. (I'm guessing tokenize just imports re.)

stdlib/3/_importlib_modulespec.pyi Outdated
@@ -9,6 +9,8 @@ from abc import ABCMeta
import sys
from typing import Any, Optional

MYPY_MODULE = 'importlib.machinery'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we go down this route, we should prefix it with _ to make clear that this name isn't actually part of this module. However, I'd prefer to just hardcode the location of ModuleType in mypy and not add special handling in typeshed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have an opinion on this, I would be happy to hard code it in mypy. Just wasn't sure if it's ok given that it's hard to find later when more circular import definitions happen.

@pkch
Copy link
Contributor Author

pkch commented Apr 3, 2017 •
edited
Loading

I replaced the MYPY_MODULE with hard-coded remapping; so this PR simply replaces module with types.ModuleType. It is blocked by python/mypy#3107, but does not block it.

@pkch pkch force-pushed the importlib-modulespec-names branch from d247130 to 04fee7c Compare April 3, 2017 01:51
@gvanrossum gvanrossum mentioned this pull request Apr 12, 2017
Merged
@pkch
Copy link
Contributor Author

pkch commented Apr 12, 2017

Superceded by #1156

@pkch pkch closed this Apr 12, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JelleZijlstra JelleZijlstra JelleZijlstra left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@pkch @JelleZijlstra