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

[flake8-type-checking] Fix false positives for typing.Annotated #14311

Merged

Conversation

Daverball
Copy link
Contributor

This partially addresses #13713

Summary

Ruff properly clears the TYPE_DEFINITION flag when visiting special parts of an annotation like typing.Literal and typing.Annotated. However there are other typing related semantic flags which aren't cleared. This is fine, however is_typing_reference only check for those higher level flags and completely ignores TYPE_DEFINITION, which can lead to false positives for TCH001-003.

Test Plan

cargo nextest run

@MichaReiser
Copy link
Member

@AlexWaygood would you mind taking a look at this PR? I think you have a better understanding of our type checking flags

Copy link
Contributor

github-actions bot commented Nov 13, 2024

ruff-ecosystem results

Linter (stable)

ℹ️ ecosystem check detected linter changes. (+3 -0 violations, +0 -0 fixes in 1 projects; 53 projects unchanged)

ibis-project/ibis (+3 -0 violations, +0 -0 fixes)

+ ibis/expr/operations/generic.py:16:44: RUF100 Unused `noqa` directive (unused: `TCH001`)
+ ibis/expr/operations/generic.py:18:54: RUF100 Unused `noqa` directive (unused: `TCH001`)
+ ibis/expr/types/groupby.py:28:42: RUF100 Unused `noqa` directive (unused: `TCH001`)

Changes by rule (1 rules affected)

code total + violation - violation + fix - fix
RUF100 3 3 0 0 0

Linter (preview)

ℹ️ ecosystem check detected linter changes. (+3 -0 violations, +0 -0 fixes in 1 projects; 53 projects unchanged)

ibis-project/ibis (+3 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview

+ ibis/expr/operations/generic.py:16:44: RUF100 Unused `noqa` directive (unused: `TCH001`)
+ ibis/expr/operations/generic.py:18:54: RUF100 Unused `noqa` directive (unused: `TCH001`)
+ ibis/expr/types/groupby.py:28:42: RUF100 Unused `noqa` directive (unused: `TCH001`)

Changes by rule (1 rules affected)

code total + violation - violation + fix - fix
RUF100 3 3 0 0 0

@Daverball
Copy link
Contributor Author

Not quite sure what the new LOG015 violations are about and how they are related to this change, but they look correct to me.

The RUF100 violations are encouraging, since these are all due to previous false positives.

@MichaReiser
Copy link
Member

@Daverball can you try rebasing/merging main. I suspect that will help with the LOG015 ecosystem changes

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.

Thank you! This LGTM.

@AlexWaygood AlexWaygood changed the title [flake8-type-checking] Fixes a false positive for typing.Annotated [flake8-type-checking] Fix false positives for typing.Annotated Nov 13, 2024
@AlexWaygood AlexWaygood merged commit 89aa804 into astral-sh:main Nov 13, 2024
20 checks passed
@Daverball Daverball deleted the bugfix/annotated-annotation-context branch November 13, 2024 12:30
@dhruvmanila
Copy link
Member

@AlexWaygood I'm assuming this is a bugfix based on the PR title but feel free to update the label if you think it's a rule change.

@dhruvmanila dhruvmanila added the bug Something isn't working label Nov 14, 2024
@dhruvmanila dhruvmanila changed the title [flake8-type-checking] Fix false positives for typing.Annotated [flake8-type-checking] Fix false positives for typing.Annotated Nov 14, 2024
@AlexWaygood
Copy link
Member

@AlexWaygood I'm assuming this is a bugfix based on the PR title but feel free to update the label if you think it's a rule change.

Yes, I think that's right! Sorry, I should have added the label.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants