-
-
Notifications
You must be signed in to change notification settings - Fork 644
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 a mypy plugin to support @total_ordering. #9525
Conversation
Without this, when using a comparison method synthesized by the `@total_ordering` class decorator, MyPy will incorrectly observe something like: ``` src/python/pants/option/option_value_container.py:108:12: error: Unsupported left operand type for >= ("Rank") [operator] if new_rank >= existing_rank: ^ Found 1 error in 1 file (checked 40 source files) ``` Here, just `__lt__` is explicitly defined in the "Rank" class and `@total_ordering` generates `__ge__` which MyPy is ignorant of. Also, in order to get the in-repo plugin working, fix the mypy task to upgrade its interpreter when `--include-requirements` is turned on and remove type checking tags from mypy example targets which are only ever used in integration tests where tags are not used as filters anyhow. [ci skip-jvm-tests]
A follow-up that converts RankedValue rank integer constants to an Enum will use this to pass type checks without compromise... besides this compromise. |
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.
Huh, I've never seen a MyPy plugin before. Neat!
Consider trying to upstream this - I think it'd be helpful to others and MyPy seems receptive to contributions like this.
@@ -32,6 +32,7 @@ def cmdline(cls, *, include_requirements): | |||
cmd = [ | |||
"--backend-packages=pants.contrib.mypy", | |||
f'--mypy-config={cls.example_dir / "mypy.ini"}', | |||
"--mypy-version=mypy==0.720", |
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.
What's the motivation for this change?
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.
This test tests the Django MyPy plugin and that plugin doesn't work with our new, newer, 3rdparty dep so I pinned to the low point of the range it does support. IIRC it was >=0.720,<0.730
.
How will this interact with the upstreaming of a potential v2 implementation of mypy? |
@Eric-Arellano it looks like this is already underway: |
Thats alot of speculation that'd I'd normally defer on until something real showed up, but I assume you're being coy about Toolchain's internal v2 impl which actually exists and is slated for upstreaming. Good question. There is a small fix in this PR for MypyTask handling of the interpreter to run mypy with when After that fix, the only other requirement - which is a requirement in v1 here too - is that the plugin registration function is available on the sys.path for MyPy to find. Since v1 adds all python source roots to the PYTHONPATH (not sure why it does this!), all v1 invocations see the plugin registration function since its in an in-repo loose source form. In v2 we'll need to add a dependency on our MyPy plugin. This will require one of:
Option 3 is what we want at the end of the day (The |
Without this, when using a comparison method synthesized by the
@total_ordering
class decorator, MyPy will incorrectly observesomething like:
Here, just
__lt__
is explicitly defined in the "Rank" class and@total_ordering
generates__ge__
which MyPy is ignorant of.Also, in order to get the in-repo plugin working, fix the mypy task to
upgrade its interpreter when
--include-requirements
is turned on andremove type checking tags from mypy example targets which are only ever
used in integration tests where tags are not used as filters anyhow.
[ci skip-jvm-tests]