-
Notifications
You must be signed in to change notification settings - Fork 27
Parser for interpreter version requirements from setup.cfg .python-version #645
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
Changes from all commits
81fa082
657af22
6f5c02e
1d59d6d
9e8a930
0024fdc
576c9d4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,6 +7,7 @@ | |
|
|
||
| import pathlib | ||
| import typing | ||
| import configparser | ||
|
|
||
| try: | ||
| import tomllib | ||
|
|
@@ -20,18 +21,38 @@ def lookup_metadata_file(directory: typing.Union[str, pathlib.Path]) -> typing.L | |
|
|
||
| The returned value is either a list of tuples [(filename, path)] or | ||
| an empty list [] if no metadata file was found. | ||
|
|
||
| The metadata files are returned in the priority they should be processed | ||
| to determine the python version requirements. | ||
| """ | ||
| directory = pathlib.Path(directory) | ||
|
|
||
| def _generate(): | ||
| for filename in ("pyproject.toml", "setup.cfg", ".python-version"): | ||
| for filename in (".python-version", "pyproject.toml", "setup.cfg"): | ||
| path = directory / filename | ||
| if path.is_file(): | ||
| yield (filename, path) | ||
|
|
||
| return list(_generate()) | ||
|
|
||
|
|
||
| def get_python_version_requirement_parser( | ||
| metadata_file: pathlib.Path, | ||
| ) -> typing.Optional[typing.Callable[[pathlib.Path], typing.Optional[str]]]: | ||
| """Given the metadata file, return the appropriate parser function. | ||
|
|
||
| The returned function takes a pathlib.Path and returns the parsed value. | ||
| """ | ||
| if metadata_file.name == "pyproject.toml": | ||
| return parse_pyproject_python_requires | ||
| elif metadata_file.name == "setup.cfg": | ||
| return parse_setupcfg_python_requires | ||
| elif metadata_file.name == ".python-version": | ||
| return parse_pyversion_python_requires | ||
| else: | ||
| return None | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
|
|
||
|
|
||
| def parse_pyproject_python_requires(pyproject_file: pathlib.Path) -> typing.Optional[str]: | ||
| """Parse the project.requires-python field from a pyproject.toml file. | ||
|
|
||
|
|
@@ -43,3 +64,27 @@ def parse_pyproject_python_requires(pyproject_file: pathlib.Path) -> typing.Opti | |
| pyproject = tomllib.loads(content) | ||
|
|
||
| return pyproject.get("project", {}).get("requires-python", None) | ||
|
|
||
|
|
||
| def parse_setupcfg_python_requires(setupcfg_file: pathlib.Path) -> typing.Optional[str]: | ||
| """Parse the options.python_requires field from a setup.cfg file. | ||
|
|
||
| Assumes that the setup.cfg file exists, is accessible and well formatted. | ||
|
|
||
| Returns None if the field is not found. | ||
| """ | ||
| config = configparser.ConfigParser() | ||
| config.read(setupcfg_file) | ||
|
|
||
| return config.get("options", "python_requires", fallback=None) | ||
|
|
||
|
|
||
| def parse_pyversion_python_requires(pyversion_file: pathlib.Path) -> typing.Optional[str]: | ||
| """Parse the python version from a .python-version file. | ||
|
|
||
| Assumes that the .python-version file exists, is accessible and well formatted. | ||
|
|
||
| Returns None if the field is not found. | ||
| """ | ||
| content = pyversion_file.read_text() | ||
| return content.strip() | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,13 @@ | ||
| import os | ||
| import pathlib | ||
|
|
||
| from rsconnect.pyproject import lookup_metadata_file, parse_pyproject_python_requires | ||
| from rsconnect.pyproject import ( | ||
| lookup_metadata_file, | ||
| parse_pyproject_python_requires, | ||
| parse_setupcfg_python_requires, | ||
| parse_pyversion_python_requires, | ||
| get_python_version_requirement_parser, | ||
| ) | ||
|
|
||
| import pytest | ||
|
|
||
|
|
@@ -11,7 +17,7 @@ | |
| # Most of this tests, verify against three fixture projects that are located in PROJECTS_DIRECTORY | ||
| # - using_pyproject: contains a pyproject.toml file with a project.requires-python field | ||
| # - using_setupcfg: contains a setup.cfg file with a options.python_requires field | ||
| # - using_pyversion: contains a .python-version file and a pyproject.toml file without any version constraint. | ||
| # - using_pyversion: contains a .python-version file and pyproject.toml, setup.cfg without any version constraint. | ||
| # - allofthem: contains all metadata files all with different version constraints. | ||
|
|
||
|
|
||
|
|
@@ -23,11 +29,12 @@ | |
| ( | ||
| os.path.join(PROJECTS_DIRECTORY, "using_pyversion"), | ||
| ( | ||
| "pyproject.toml", | ||
| ".python-version", | ||
| "pyproject.toml", | ||
| "setup.cfg", | ||
| ), | ||
| ), | ||
| (os.path.join(PROJECTS_DIRECTORY, "allofthem"), ("pyproject.toml", "setup.cfg", ".python-version")), | ||
| (os.path.join(PROJECTS_DIRECTORY, "allofthem"), (".python-version", "pyproject.toml", "setup.cfg")), | ||
| ], | ||
| ids=["pyproject.toml", "setup.cfg", ".python-version", "allofthem"], | ||
| ) | ||
|
|
@@ -37,6 +44,23 @@ def test_python_project_metadata_detect(project_dir, expected): | |
| assert lookup_metadata_file(project_dir) == expectation | ||
|
|
||
|
|
||
| @pytest.mark.parametrize( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe consider a comment describing this test?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is one at line 58, do think it's not sufficiently informative?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, sorry, I caught myself seeing that for a different test and suppressed my comment. This is fine. |
||
| "filename, expected_parser", | ||
| [ | ||
| ("pyproject.toml", parse_pyproject_python_requires), | ||
| ("setup.cfg", parse_setupcfg_python_requires), | ||
| (".python-version", parse_pyversion_python_requires), | ||
| ("invalid.txt", None), | ||
| ], | ||
| ids=["pyproject.toml", "setup.cfg", ".python-version", "invalid"], | ||
| ) | ||
| def test_get_python_version_requirement_parser(filename, expected_parser): | ||
| """Test that given a metadata file name, the correct parser is returned.""" | ||
| metadata_file = pathlib.Path(PROJECTS_DIRECTORY) / filename | ||
| parser = get_python_version_requirement_parser(metadata_file) | ||
| assert parser == expected_parser | ||
|
|
||
|
|
||
| @pytest.mark.parametrize( | ||
| "project_dir", | ||
| [ | ||
|
|
@@ -59,9 +83,43 @@ def test_python_project_metadata_missing(project_dir): | |
| ids=["option-exists", "option-missing"], | ||
| ) | ||
| def test_pyprojecttoml_python_requires(project_dir, expected): | ||
| """Test that the python_requires field is correctly parsed from pyproject.toml. | ||
| """Test that the requires-python field is correctly parsed from pyproject.toml. | ||
|
|
||
| Both when the option exists or when it missing in the pyproject.toml file. | ||
| """ | ||
| pyproject_file = pathlib.Path(project_dir) / "pyproject.toml" | ||
| assert parse_pyproject_python_requires(pyproject_file) == expected | ||
|
|
||
|
|
||
| @pytest.mark.parametrize( | ||
| "project_dir, expected", | ||
| [ | ||
| (os.path.join(PROJECTS_DIRECTORY, "using_setupcfg"), ">=3.8"), | ||
| (os.path.join(PROJECTS_DIRECTORY, "using_pyversion"), None), | ||
| ], | ||
| ids=["option-exists", "option-missing"], | ||
| ) | ||
| def test_setupcfg_python_requires(tmp_path, project_dir, expected): | ||
| """Test that the python_requires field is correctly parsed from setup.cfg. | ||
|
|
||
| Both when the option exists or when it missing in the file. | ||
| """ | ||
| setupcfg_file = pathlib.Path(project_dir) / "setup.cfg" | ||
| assert parse_setupcfg_python_requires(setupcfg_file) == expected | ||
|
|
||
|
|
||
| @pytest.mark.parametrize( | ||
| "project_dir, expected", | ||
| [ | ||
| (os.path.join(PROJECTS_DIRECTORY, "using_pyversion"), ">=3.8, <3.12"), | ||
| ], | ||
| ids=["option-exists"], | ||
| ) | ||
| def test_pyversion_python_requires(tmp_path, project_dir, expected): | ||
| """Test that the python version is correctly parsed from .python-version. | ||
|
|
||
| We do not test the case where the option is missing, as an empty .python-version file | ||
| is not a valid case for a python project. | ||
| """ | ||
| versionfile = pathlib.Path(project_dir) / ".python-version" | ||
| assert parse_pyversion_python_requires(versionfile) == expected | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| [metadata] | ||
| name = python-project | ||
| version = 0.1.0 | ||
| description = Add your description here |
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.
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:
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.
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.tomllowercase, 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, thanks for considering my suggestion.