Use modern rules_python and expect users to provide the mypy library themselves #98
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
We don't use `maybe` for py_repositories.bzl as maybe can't be invoked recursively and the functions provided by rules_python already use it internally
While using a variable to construct the URLs is the approach with lower maintenance, using plain strings is required to allow Renovate to understand them.
This is a recent version supporting Bazel 6 and the forward path for managing external dependencies based on `pip_parse`. We don't use the latest version as rules_python >= 0.19.0 is no longer compatible to Bazel 4.x and it might still be too early for some users of this project for this step. Noteworthy changes: - Use `pip_parse` instead of deprecated `pip_install`. Consequently, users now have to provide a fully resolved requirements file for mypy. This means, there is no more default mypy shipped with this project. Users always have to provide their own mypy. The mypy tracked in third_party is used solely for testing. - We use a hermetic Python 3.8 toolchain from rules_python. Strictly speaking a resolved requirements file is only valid for the Python interpreter and the platform it was created for. As far as we now mypy has no platform specific dependencies. And even if this changes, rules_python offers a feature for registering platform specific requirement files. However, some mypy versions have dependencies which are only valid for specific Python versions (e.g. `typed-ast`). To ensure the project behaves the same for each developer, we use a hermetic Python toolchain fixing the Python version independent from the Python interpreter on the developer host.
Users are expected to provide mypy as this project cannot provide a version which matches all possible user use cases. Until now users had to provide a resolved requirements file. This is however an unnecessarily complex model. It exposes to the user that this projects uses rules_python to manage its dependencies in a specific way. The more natural solution is that we expect the user to provide a mypy library matching the users Python toolchain and expectations. On top this eases integrating this project into other workspaces as less interaction via macros is required.
Using the internal testing mypy as default for the mypy label_flag forces users to resolve `mypy_integration_pip_deps` which is not supposed to be relevant for them. To prevent this we set an empty library as default for the mypy label_flag to ensure users provide their own mypy and to decouple them from `mypy_integration_pip_deps`.
adzenith
approved these changes
Feb 12, 2024
README.md
Outdated
| this file to the `deps()` function in `@mypy_integration//repositories:deps.bzl`, which below is named | ||
| `mypy_integration_deps(...)`: | ||
| `mypy_integration` expects the user to provide the `mypy` dependency. | ||
| Given not every mypy version is compatible to all Python versions and mypy's transitive dependencies can differ based on the Python version `mypy_integration` cannot offer a `mypy` which satisfies all potential users. |
There was a problem hiding this comment.
Suggested change
| Given not every mypy version is compatible to all Python versions and mypy's transitive dependencies can differ based on the Python version `mypy_integration` cannot offer a `mypy` which satisfies all potential users. | |
| Given not every mypy version is compatible to all Python versions and mypy's transitive dependencies can differ based on the Python version, `mypy_integration` cannot offer a `mypy` which satisfies all potential users. |
There was a problem hiding this comment.
Will fix this in another PR so this large PR can be merged without a new review.
I misinterpreted the green status as merge ready. Will fix this asap.
There was a problem hiding this comment.
Ah right sorry, at work when I approve a PR the author hits merge when they're ready, so here out of habit I approved and then didn't hit merge. I'm happy to merge whenever you're ready.
|
thank you!! |
This was referenced Feb 20, 2024
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.
New version of #82
As requested, rules_python is now the latest version still supporting Bazel 5.
There is still no default version for
mypy, as the discussion about this is still ongoing.I still believe there should be no default version.