Remove a same-variable-name class hierarchy #1092
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The modified definition makes it possible to use `pkg_resources` with `mypy`, since `mypy` currently fails since it detects a cycle in the class's hierarchy. Additionally, this also makes the intent slightly clearer IMO.
|
My initial instinct is that Setuptools shouldn't be adopting a workaround for a defect in another system without a clear acknowledgement that it's a workaround. I don't yet understand how the current implementation is deficient or how changing it fixes things. Can you provide a distilled, minimal example of why the current behavior is problemmatic? I tried using _get_mro with mypy installed and I had no problems: But I can't tell from the mypy docs how to activate it or use it. I do think that the change you're proposing here looks sound. I'd just like to have an understanding why it's superior before accepting it. |
Running mypy with mypy thinks there's a cycle in the class hierarchy, at the class definition line (that has been changed in this PR) since the new class refers to the old class and has the same name. By changing the name of the new class, this issue is worked around. I felt that this change was too trivial to deserve a comment in the sources. |
If you still want one after the previous post, I'll happily look into make a reproducible example. :)
That's not how to trigger this. mypy refusing to check types on anything that refers to pkg_resources through an import - because it detects a cycle in the class hierarchy. |
|
Here's running mypy on a file importing pkg_resources: I do see an error when running against pkg_resources package itself. |
|
Applying the patch, however, I get the same result. |
|
Perhaps that _get_mro issue only occurs when an old-style class is present, which would only happen on Python 2. |
|
As I look at it, I see there's another _get_mro function in setuptools.monkey, which does mostly the same thing, but addresses different issues. Probably these functions need to be consolidated. |
mypy only runs on Py3. |
jaraco
added a commit
that referenced
this pull request
Jul 26, 2017
|
I've pushed an alternate approach in 35fa31f. |
|
It's way past midnight - I'll let you know if this works for the motivating PR tomorrow evening. :) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
pypa/pip#4545 is an attempt to add mypy type-checking to pip. In that PR, the vendored version of
pkg_resourcesis patched (with this same change) since it is needed formypyto run on the codebase.This patch is really a workaround for mypy's crashing behaviour; as can be seen in https://travis-ci.org/pypa/pip/jobs/243330633#L120.
It would be nice if pip's vendoring task wouldn't have to do such a thing, given the triviality of this change.
@jaraco Thoughts?