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-0.8] [FAST] Further improve docs for fast-api-non-annotated-depencency (FAST002) #14467

Merged
merged 1 commit into from
Nov 20, 2024

Conversation

AlexWaygood
Copy link
Member

This PR is stacked on top of #14466.

Summary

This PR updates the docs for FAST002 to note that typing.Annotated was added in Python 3.9, but that typing_extensions.Annotated can be used on older versions of Python.

Alternatively, we could consider reverting the stabilisation of this rule, since not everybody will necessarily be happy to add typing_extensions as a dependency, but the autofix assumes that it is available to be imported on Python 3.8 and lower. However, nobody has raised this issue, and Python 3.8 is now end-of-life, so while this isn't ideal I think it probably shouldn't block the rule being stabilised. If you are on an older version of Python and don't want typing_extensions as a dependency, you can always just disable the rule.

I don't have a strong opinion here, though; this does feel sort-of borderline to me. Maybe we should revert the stabilisation?

Test Plan

cargo test -p ruff_linter

@AlexWaygood AlexWaygood added the documentation Improvements or additions to documentation label Nov 19, 2024
@AlexWaygood AlexWaygood added this to the v0.8 milestone Nov 19, 2024
@AlexWaygood AlexWaygood force-pushed the fast002-preview-again branch from 72aee00 to e8ce962 Compare November 19, 2024 18:32
Copy link
Contributor

github-actions bot commented Nov 19, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

@AlexWaygood
Copy link
Member Author

Thinking about this some more, I think we should at least mark the autofix as unsafe if the target version is set to Python 3.8 or lower, since the autofix adds imports of typing_extensions on these older Python versions, which the user might not have or want as a dependency. So the fix could easily break working Python code on older Python versions.

Copy link
Member

@MichaReiser MichaReiser left a comment

Choose a reason for hiding this comment

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

Let's say we hold back on stabilizing this rule now because of Python 3.8. How would you change the rule or what are the next steps to stabilize this rule in an upcoming release?

To me it's unclear what that change would be:

  • We could only flag the rule for Python 3.9 or newer, but this makes the rule useless for users targeting Python 3.8 that want the rule
  • We could introduce an option, but an option is equivalent to toggling the rule
  • We could test if a typing module is already imported. I don't think this will be very useful unless it's done globally and not on a per file level, which Ruff doesn't support.

That's why think we should stabilize the rule. Users that don't want typing rules can disable the rule.

@MichaReiser
Copy link
Member

And nice find!

Base automatically changed from alex/stabilised-docs to ruff-0.8 November 20, 2024 08:00
@AlexWaygood AlexWaygood force-pushed the fast002-preview-again branch from b66d9b6 to b4015b0 Compare November 20, 2024 10:39
@AlexWaygood
Copy link
Member Author

Thinking about this some more, I think we should at least mark the autofix as unsafe if the target version is set to Python 3.8 or lower, since the autofix adds imports of typing_extensions on these older Python versions, which the user might not have or want as a dependency. So the fix could easily break working Python code on older Python versions.

Okay, this isn't an issue, because the fix is already marked as unsafe on all Python versions. However, there's no ## Fix safety section in the documentation explaining why this is so. I also can't see any explicit discussion of whether the fix should be safe or unsafe in #11579 :/

@AlexWaygood
Copy link
Member Author

Landing for now; this is an improvement on the status quo, and we can always add more docs on the safety of the fix as a followup

@AlexWaygood AlexWaygood merged commit 52c2e40 into ruff-0.8 Nov 20, 2024
17 checks passed
@AlexWaygood AlexWaygood deleted the fast002-preview-again branch November 20, 2024 10:48
@AlexWaygood
Copy link
Member Author

I opened #14484

MichaReiser pushed a commit that referenced this pull request Nov 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants