Parser for interpreter version requirements from setup.cfg .python-version #645
Conversation
☂️ Python Coverage
Overall Coverage
New FilesNo new covered files... Modified Files
|
amol-
force-pushed
the
extract-setupcfg-pyversion-30404-30405
branch
from
588e136 to
9e8a930
Compare
| elif metadata_file.name == ".python-version": | ||
| return parse_pyversion_python_requires | ||
| else: | ||
| return None |
There was a problem hiding this comment.
This could be done with a lookup table, but given the options aren't many I preferred to be explicit and avoid dynamic lookups which are harder to follow when debugging.
sagerb
left a comment
sagerb
left a comment
There was a problem hiding this comment.
The code is looking great. I provided a few small suggestions which I think will improve the code. Reach out when you are ready for another review.
| The returned function takes a pathlib.Path and returns the parsed value. | ||
| """ | ||
| if metadata_file.name == "pyproject.toml": |
There was a problem hiding this comment.
I'm always wondering if we should be comparing filenames with a case-insensitive comparison. When I looked, google's AI provided this, so I think this might be a good improvement here:
The pyproject.toml filename is generally treated as case-insensitive by most Python tools and systems. While the file system itself might be case-sensitive, tools that interact with pyproject.toml, such as pip, poetry, and build, typically perform case-insensitive matching when searching for the file.
There was a problem hiding this comment.
Given
https://github.com/pypa/pip/blob/main/src/pip/_internal/req/req_install.py#L512-L514
and
https://github.com/pypa/pip/blob/main/src/pip/_internal/pyproject.py#L26-L27
I think it's fairly safe to assume that the filename is expected to be lowercase for pip too.
It's true that the PEP doesn't explicitly say that the filename must be case sensitive, but in every single place it explicitly write pyproject.toml lowercase, which I think it's safe to expect it means the filename is expected to always be lowercase and thus work in both case sensitive and case insensitive file systems.
There was a problem hiding this comment.
ok, thanks for considering my suggestion.
| assert lookup_metadata_file(project_dir) == expectation | ||
|
|
||
|
|
||
| @pytest.mark.parametrize( |
There was a problem hiding this comment.
Maybe consider a comment describing this test?
There was a problem hiding this comment.
There is one at line 58, do think it's not sufficiently informative?
There was a problem hiding this comment.
Yes, sorry, I caught myself seeing that for a different test and suppressed my comment. This is fine.
sagerb
left a comment
sagerb
left a comment
There was a problem hiding this comment.
Thanks for responding to my comments. I'm good with this now.
Intent
Second step toward supporting python interpreter version requirements in
manifest.jsongeneration.This is a follow-up of #643 that adds support for the other two planned file formats.
Type of Change
Approach
Continuing with small incremental PRs approach to add support for each new format keeping the review effort minimal.
Automated Tests
New tests were added for every function in the
test_pyproject.pyfile.Directions for Reviewers
This continues in the direction of building all the necessary blocks without exposing the feature to the end user until the very end. The PR is based on the #643 code, so it should be reviewed only once the other PR is merged as it will influence this one.