Skip to content
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

[red-knot] Fallback to requires-python if no python-version is specified #16028

Merged
merged 5 commits into from
Feb 12, 2025

Conversation

MichaReiser
Copy link
Member

@MichaReiser MichaReiser commented Feb 7, 2025

Summary

Add support for the project.requires-python field in pyproject.toml files.

Fall back to the resolved lower bound of project.requires-python if the environment.python-version field is None (or more accurately, initialize environment.python-version with requires-python`'s lower bound if left unspecified).

UX design

There are two options on how we can handle the fallback to requires-python's lower bound:

  1. Store the resolved lower bound in environment.python-version if that field is None (Implemented in this PR)
  2. Store the requires-python constraint separately.

There's no observed difference unless a user-level configuration (or any other inherited configuration is used). Let's discuss it on the given example

User configuration

[environment]
python-version = "3.10"

Project configuration (pyproject.toml)

[project]
name = "test"
requires-python = ">= 3.12"

[tool.knot]
# No environment table

The resolved version for 1. is 3.12 because the requires-python constraint precedence takes precedence over the python-version in the user configuration. 2. resolves to 3.10 because all python-version constraints take precedence before falling back to requires-python.

Ruff implements 1. It's also the easier to implement and it does seem intuitive to me that the more local requires-python constraint takes precedence.

Test plan

Added CLI and unit tests.

@MichaReiser MichaReiser force-pushed the micha/user-level-configuration branch from b2aeb58 to dd75dc2 Compare February 8, 2025 16:17
@MichaReiser MichaReiser force-pushed the micha/requires-python branch from fad3730 to a2f8e8e Compare February 8, 2025 16:29

tracing::debug!("Resolving requires-python constraint: `{requires_python}`");

let ranges = release_specifiers_to_ranges((&**requires_python).clone());
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks to @konstin for pointing me toward the logic in uv. This matches what uv does. Thankfully, most of the work is done by the pep0440 crate.

@MichaReiser MichaReiser changed the title Fallback to requires python if no python-version is specified [red-knot] Fallback to requires python if no python-version is specified Feb 8, 2025
@MichaReiser MichaReiser added configuration Related to settings and configuration red-knot Multi-file analysis & type inference labels Feb 8, 2025
@MichaReiser MichaReiser force-pushed the micha/requires-python branch from 7614b61 to 3dc7900 Compare February 8, 2025 16:41
@MichaReiser MichaReiser marked this pull request as ready for review February 8, 2025 16:41
@MichaReiser MichaReiser force-pushed the micha/requires-python branch from 3dc7900 to bd1f597 Compare February 8, 2025 16:43
@MichaReiser MichaReiser changed the title [red-knot] Fallback to requires python if no python-version is specified [red-knot] Fallback to requires-python if no python-version is specified Feb 8, 2025
@MichaReiser MichaReiser force-pushed the micha/user-level-configuration branch from 4355d5d to 521cc67 Compare February 10, 2025 14:53
Base automatically changed from micha/user-level-configuration to main February 10, 2025 15:44
@MichaReiser MichaReiser force-pushed the micha/requires-python branch 3 times, most recently from bd1f597 to 522bdb5 Compare February 10, 2025 15:55
Copy link
Contributor

@sharkdp sharkdp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you. A few minor comments and one higher-level question.

crates/red_knot_project/src/metadata.rs Outdated Show resolved Hide resolved
crates/red_knot_project/src/metadata.rs Show resolved Hide resolved
crates/red_knot_project/src/metadata.rs Outdated Show resolved Hide resolved
crates/red_knot_project/src/metadata.rs Outdated Show resolved Hide resolved
@MichaReiser MichaReiser force-pushed the micha/requires-python branch from 522bdb5 to d695c58 Compare February 12, 2025 10:15
@MichaReiser MichaReiser force-pushed the micha/requires-python branch from d695c58 to 8aae983 Compare February 12, 2025 10:18
Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

crates/red_knot_project/src/metadata/pyproject.rs Outdated Show resolved Hide resolved
crates/red_knot_project/src/metadata.rs Outdated Show resolved Hide resolved
@MichaReiser MichaReiser enabled auto-merge (squash) February 12, 2025 11:44
@MichaReiser MichaReiser merged commit 03f0828 into main Feb 12, 2025
20 checks passed
@MichaReiser MichaReiser deleted the micha/requires-python branch February 12, 2025 11:48
@@ -121,7 +119,7 @@ pub enum ResolveRequiresPythonError {
TooLargeMajor(u64),
#[error("The minor version `{0}` is larger than the maximum supported value 255")]
TooLargeMinor(u64),
#[error("value `{0}` does not contain a lower bound. Add a lower bound to indicate the minimum compatible Python version (e.g., `>=3.13`).")]
#[error("value `{0}` does not contain a lower bound. Add a lower bound to indicate the minimum compatible Python version (e.g., `>=3.13`) or specify a version in `environment.python-version`.")]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They might not have a tool.knot table at all, right? This could be quite confusing if they aren't familiar with how to provide this red-knot-specific setting

Suggested change
#[error("value `{0}` does not contain a lower bound. Add a lower bound to indicate the minimum compatible Python version (e.g., `>=3.13`) or specify a version in `environment.python-version`.")]
#[error("value `{0}` does not contain a lower bound. Add a lower bound to indicate the minimum compatible Python version (e.g., `>=3.13`) or specify a version in `tool.knot.environment.python-version`.")]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
configuration Related to settings and configuration red-knot Multi-file analysis & type inference
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants