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] Consider type expressions in list for quoting annotations #14371

Merged
merged 3 commits into from
Nov 16, 2024

Conversation

dylwil3
Copy link
Collaborator

@dylwil3 dylwil3 commented Nov 15, 2024

This PR adds corrected handling of list expressions to the Visitor implementation of QuotedAnnotator in flake8_type_checking::helpers.

Closes #14368

@dylwil3
Copy link
Collaborator Author

dylwil3 commented Nov 15, 2024

Notice that the snapshot I updated was for a test that already existed - the fixture contains almost exactly the example from the linked issue. It seems to me like the previous behavior was incorrect and the new behavior is correct - but maybe I'm missing something?

Copy link
Contributor

github-actions bot commented Nov 15, 2024

ruff-ecosystem results

Linter (stable)

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

pandas-dev/pandas (+0 -0 violations, +0 -0 fixes)


python-trio/trio (+2 -0 violations, +0 -0 fixes)

+ src/trio/testing/_memory_streams.py:438:1: W293 Blank line contains whitespace
+ src/trio/testing/_memory_streams.py:438:2: W292 No newline at end of file

Changes by rule (2 rules affected)

code total + violation - violation + fix - fix
W293 1 1 0 0 0
W292 1 1 0 0 0

Linter (preview)

✅ ecosystem check detected no linter changes.

@dylwil3 dylwil3 marked this pull request as ready for review November 16, 2024 01:41
@MichaReiser MichaReiser added the bug Something isn't working label Nov 16, 2024
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.

This looks correct to me. I don't know anything about helpers.rs but you're doing the same as is done in other places, so it must be correct 😆

@charliermarsh charliermarsh merged commit fc392c6 into astral-sh:main Nov 16, 2024
20 checks passed
dylwil3 added a commit to dylwil3/ruff that referenced this pull request Nov 17, 2024
…ons in quotes (astral-sh#14371)

This PR adds corrected handling of list expressions to the `Visitor`
implementation of `QuotedAnnotator` in `flake8_type_checking::helpers`.

Closes astral-sh#14368
@dhruvmanila dhruvmanila changed the title [flake8-type-checking] Fix helper function which surrounds annotations in quotes [flake8-type-checking] Consider type expressions in list for quoting annotations Nov 18, 2024
dhruvmanila added a commit that referenced this pull request Nov 18, 2024
## Summary

Follow-up to #14371, this PR simplifies the visitor logic for list
expressions to remove the state management. We just need to make sure
that we visit the nested expressions using the `QuoteAnnotator` and not
the `Generator`. This is similar to what's being done for binary
expressions.

As per the
[grammar](https://typing.readthedocs.io/en/latest/spec/annotations.html#grammar-token-expression-grammar-annotation_expression),
list expressions can be present which can contain other type expressions
(`Callable`):
```
       | <Callable> '[' <Concatenate> '[' (type_expression ',')+
                    (name | '...') ']' ',' type_expression ']'
             (where name must be a valid in-scope ParamSpec)
       | <Callable> '[' '[' maybe_unpacked (',' maybe_unpacked)*
                    ']' ',' type_expression ']'
```

## Test Plan

`cargo insta test`
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.

TCH fixes remove quotation marks from Literal strings
3 participants