Fix the mypy-protobuf interpreter constraint. #11695
Conversation
Also add an option to allow a user to adjust the interpreter constraint when adjusting the mypy-protobuf plugin version as they can with other python tools. Fixes pantsbuild#11565 [ci skip-rust] [ci skip-build-wheels]
This brings in new features, including the option to generating gRPC type stubs.
# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
| # the deprecated option in flag and environment variable namespaces; i.e.: both | ||
| # --python-protobuf-mypy-plugin-version and PANTS_PYTHON_PROTOBUF_MYPY_PLUGIN_VERSION | ||
| # will continue to work." | ||
| deprecated_conditional( |
There was a problem hiding this comment.
I'm not sure that I fully understand how this deprecation works, but given that the plugin was added recently, I won't investigate too closely.
There was a problem hiding this comment.
Yeah, it's tricky enough that I went through the steps to test all this. The only deprecated thing is the config syntax and this triggers only for that by design.
| def version(self) -> str: | ||
| def requirement(self) -> str: |
| predicate=lambda: ( | ||
| python_protobuf_subsystem.options.get_rank(deprecated_key) == Rank.CONFIG | ||
| ), |
There was a problem hiding this comment.
Typically we use not subsystem.options.is_default("foo") - is there a particular reason to only check for config?
There was a problem hiding this comment.
Even better, you can deprecate the original --mypy-plugin-version option, right? That is less code, makes the deprecation be eager at Pants init, and updates ./pants help + the site to do the right thing.
There was a problem hiding this comment.
Config is the only deprecated thing. The flag and env var forms do not change as the comment tries to make clear. As such, to deprecate any non-config usage would be to complain at a user for doing nothing wrong.
| return cast(str, self.options.mypy_plugin_version) | ||
|
|
||
| class PythonProtobufMypyPlugin(PythonToolRequirementsBase): | ||
| options_scope = PythonProtobufSubsystem.subscope("mypy-plugin") |
There was a problem hiding this comment.
FYI this is the first usage of subscopes in Pants 2.x. I think we had some tentative discussion a few months back on whether we want to have this or not, and what the semantics should be. Cc @stuhood, you had some thoughts iirc.
I think this is fine, but want to draw attention to this setting up a new precedent.
There was a problem hiding this comment.
I think that this usecase is roughly in the spirit of how we would want subscopes to be used. But honestly, I'm mostly surprised that it works.
@jsirois: The primary difference to watch out for is that in the past, subscopes were primarily generated dynamically to add a common/reusable Subsystem to a scope. Because of that, we have inheritance/recursion of options applied to scopes. For example: with a scope my-jvm-tool.common-jvm-config, setting an option in scope my-jvm-tool.common-jvm-config affects only that instance of the subsystem. But setting that same option in common-jvm-config would affect all subscopes like *.common-jvm-config.
There was a problem hiding this comment.
Yeah - I wrote that bit of Subsystems IIRC. This does not use that though, it just uses the subscope namespace as a trick to ensure shadowing as opposed to setting the subsystem options scope to python-protobuf-mypy-plugin which I also could have done. The difference would have merely been s/./-/ in the pants.toml config key name (i.e.: s/[python-protobuf.mypy-plugin]/[python-protobuf-mypy-plugin]/. The s/./-/ does not affect either the flag name or the env var name.
| @@ -43,7 +46,6 @@ def register_options(cls, register): | |||
| register( | |||
There was a problem hiding this comment.
Thoughts on moving the above --mypy-plugin bool option to the new subsystem? I'm not sure which place make more sense.
There was a problem hiding this comment.
I think it makes more sense here. Do you want extra feature foo? Great, I'll load a subsystem for that to get the configuration for the extra feature. The use-it-ness is a property of the python-protobuf configuration.
There was a problem hiding this comment.
That sounds sensible to me. Thanks!
Also add an option to allow a user to adjust the interpreter constraint
when adjusting the mypy-protobuf plugin version as they can with other
python tools.
Fixes #11565
[ci skip-rust]
[ci skip-build-wheels]