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

Ruff target-version can be automatically inferred if requires-python is set #201

Closed
burgholzer opened this issue Jul 2, 2023 · 5 comments · Fixed by #205
Closed

Ruff target-version can be automatically inferred if requires-python is set #201

burgholzer opened this issue Jul 2, 2023 · 5 comments · Fixed by #205

Comments

@burgholzer
Copy link
Contributor

burgholzer commented Jul 2, 2023

According to https://beta.ruff.rs/docs/settings/#target-version, the target-version property used for ruff can be inferred from the project.requires-python field, if it is present.
Repo-review currently requires the target-version to be set in

class RF002(Ruff):
"Target version must be set"
requires = {"RF001"}
@staticmethod
def check(pyproject: dict[str, Any]) -> bool:
"""
Must select a minimum version to target. Affects pyupgrade,
isort, and others.
"""
match pyproject:
case {"tool": {"ruff": {"target-version": str()}}}:
return True
case _:
return False

Based on the above, this check could be relaxed a little. Maybe to something like:

class RF002(Ruff):
    "Target version must be set"
    requires = {"RF001"}

    @staticmethod
    def check(pyproject: dict[str, Any]) -> bool:
        """
        Must select a minimum version to target. Affects pyupgrade,
        isort, and others. Can be inferred from project.requires-python.
        """

        match pyproject:
            case {"tool": {"ruff": {"target-version": str()}}}:
                return True
            case {"project": {"requires-python": str()}}:
                return True
            case _:
                return False

Or even something where it is suggested to not specify tool.ruff.target-version whenever project.requires-python is specified.

@henryiii
Copy link
Collaborator

henryiii commented Jul 2, 2023

This sounds great. I’d go with allowing either for now, as you have above, and later we could make having both disallowed.

@henryiii
Copy link
Collaborator

henryiii commented Jul 2, 2023

(I suspect this might be somewhat new. Also wondering how it handles more complex version boudaries)

@burgholzer
Copy link
Contributor Author

This sounds great. I’d go with allowing either for now, as you have above, and later we could make having both disallowed.

Do you want me to submit a corresponding PR?

(I suspect this might be somewhat new. Also wondering how it handles more complex version boudaries)

The feature was requested in astral-sh/ruff#2039 and implemented in astral-sh/ruff#3470 on March 13, which went into ruff v0.0.255 (https://github.com/astral-sh/ruff/releases/tag/v0.0.255). astral-sh/ruff#2039 has some details on the handling of version ranges (mostly refers to how black handles this as of version 23.1.0).

@henryiii
Copy link
Collaborator

henryiii commented Jul 2, 2023

Sure, please do.

@henryiii
Copy link
Collaborator

henryiii commented Jul 3, 2023

(FYI, this check was added in repo-review 0.5, https://github.com/scientific-python/repo-review/releases/tag/v0.5.0 - in February, which explains why it didn't have this :) )

matthewfeickert pushed a commit to scikit-hep/pyhf that referenced this issue Jul 3, 2023
* Remove target-version from Black and Ruff metadata in pyproject.toml.
   - c.f. scientific-python/cookie#201
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants