Update ISC001, ISC002 to check in f-strings
#7515
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This was referenced Sep 19, 2023
dhruvmanila
added
rule
dhruvmanila
commented
Sep 19, 2023
Comment on lines
+64
to
+69
| _ = f"a {'b' 'c' 'd'} e" | ||
| _ = f"""abc {"def" "ghi"} jkl""" | ||
| _ = f"""abc { | ||
| "def" | ||
| "ghi" | ||
| } jkl""" |
There was a problem hiding this comment.
We didn't detect this in earlier version but with the granularity of the tokens, we can detect this as well.
CodSpeed Performance ReportMerging #7515 will not alter performanceComparing Summary
|
dhruvmanila
force-pushed
the
dhruv/pep-701
branch
from
db4992a
to
101f9be
Compare
dhruvmanila
force-pushed
the
dhruv/issue-7298
branch
from
d559b0a
to
fe3eced
Compare
MichaReiser
approved these changes
Sep 20, 2023
| (Tok::String { .. }, Tok::String { .. }) => (*a_range, *b_range), | ||
| (Tok::String { .. }, Tok::FStringStart) => ( | ||
| *a_range, | ||
| indexer.fstring_ranges().innermost(b_range.start()).unwrap(), |
There was a problem hiding this comment.
I find the use of fstring_ranges here a bit unfortunate because:
- Looking up the range is at least:
O(log(n)) - So many unwraps
But I guess it's fine, considering that implicit concatenated strings are somewhat rare
There was a problem hiding this comment.
Yes, I agree. This could be solved with the introduction of a new StringList node though as the AST would contain the range, so no lookups and no unwraps 😉
dhruvmanila
added a commit
that referenced
this pull request
Sep 20, 2023
## Summary This PR updates the implicit string concatenation rules, specifically `ISC001` and `ISC002` to account for the new f-string tokens. `ISC003` checks for explicit string concatenation and is not affected by PEP 701 because it is based on AST. ### Implementation The implementation is based on the boundary tokens of the f-string which are `FStringStart` and `FStringEnd`. There are 4 cases to look for: 1. `String` followed by `FStringStart` 2. `FStringEnd` followed by `String` 3. `FStringEnd` followed by `FStringStart` 4. `String` followed by `String` For f-string tokens, we use the `Indexer` to get the entire range of the f-string. This is the range of the innermost f-string. ## Test Plan Add new test cases for nested f-strings.
dhruvmanila
added a commit
that referenced
this pull request
Sep 21, 2023
## Summary This PR updates the implicit string concatenation rules, specifically `ISC001` and `ISC002` to account for the new f-string tokens. `ISC003` checks for explicit string concatenation and is not affected by PEP 701 because it is based on AST. ### Implementation The implementation is based on the boundary tokens of the f-string which are `FStringStart` and `FStringEnd`. There are 4 cases to look for: 1. `String` followed by `FStringStart` 2. `FStringEnd` followed by `String` 3. `FStringEnd` followed by `FStringStart` 4. `String` followed by `String` For f-string tokens, we use the `Indexer` to get the entire range of the f-string. This is the range of the innermost f-string. ## Test Plan Add new test cases for nested f-strings.
dhruvmanila
added a commit
that referenced
this pull request
Sep 22, 2023
## Summary This PR updates the implicit string concatenation rules, specifically `ISC001` and `ISC002` to account for the new f-string tokens. `ISC003` checks for explicit string concatenation and is not affected by PEP 701 because it is based on AST. ### Implementation The implementation is based on the boundary tokens of the f-string which are `FStringStart` and `FStringEnd`. There are 4 cases to look for: 1. `String` followed by `FStringStart` 2. `FStringEnd` followed by `String` 3. `FStringEnd` followed by `FStringStart` 4. `String` followed by `String` For f-string tokens, we use the `Indexer` to get the entire range of the f-string. This is the range of the innermost f-string. ## Test Plan Add new test cases for nested f-strings.
dhruvmanila
added a commit
that referenced
this pull request
Sep 22, 2023
## Summary This PR updates the implicit string concatenation rules, specifically `ISC001` and `ISC002` to account for the new f-string tokens. `ISC003` checks for explicit string concatenation and is not affected by PEP 701 because it is based on AST. ### Implementation The implementation is based on the boundary tokens of the f-string which are `FStringStart` and `FStringEnd`. There are 4 cases to look for: 1. `String` followed by `FStringStart` 2. `FStringEnd` followed by `String` 3. `FStringEnd` followed by `FStringStart` 4. `String` followed by `String` For f-string tokens, we use the `Indexer` to get the entire range of the f-string. This is the range of the innermost f-string. ## Test Plan Add new test cases for nested f-strings.
This was referenced Sep 22, 2023
dhruvmanila
added a commit
that referenced
this pull request
Sep 22, 2023
## Summary This PR updates the implicit string concatenation rules, specifically `ISC001` and `ISC002` to account for the new f-string tokens. `ISC003` checks for explicit string concatenation and is not affected by PEP 701 because it is based on AST. ### Implementation The implementation is based on the boundary tokens of the f-string which are `FStringStart` and `FStringEnd`. There are 4 cases to look for: 1. `String` followed by `FStringStart` 2. `FStringEnd` followed by `String` 3. `FStringEnd` followed by `FStringStart` 4. `String` followed by `String` For f-string tokens, we use the `Indexer` to get the entire range of the f-string. This is the range of the innermost f-string. ## Test Plan Add new test cases for nested f-strings.
dhruvmanila
added a commit
that referenced
this pull request
Sep 26, 2023
## Summary This PR updates the implicit string concatenation rules, specifically `ISC001` and `ISC002` to account for the new f-string tokens. `ISC003` checks for explicit string concatenation and is not affected by PEP 701 because it is based on AST. ### Implementation The implementation is based on the boundary tokens of the f-string which are `FStringStart` and `FStringEnd`. There are 4 cases to look for: 1. `String` followed by `FStringStart` 2. `FStringEnd` followed by `String` 3. `FStringEnd` followed by `FStringStart` 4. `String` followed by `String` For f-string tokens, we use the `Indexer` to get the entire range of the f-string. This is the range of the innermost f-string. ## Test Plan Add new test cases for nested f-strings.
dhruvmanila
added a commit
that referenced
this pull request
Sep 27, 2023
## Summary This PR updates the implicit string concatenation rules, specifically `ISC001` and `ISC002` to account for the new f-string tokens. `ISC003` checks for explicit string concatenation and is not affected by PEP 701 because it is based on AST. ### Implementation The implementation is based on the boundary tokens of the f-string which are `FStringStart` and `FStringEnd`. There are 4 cases to look for: 1. `String` followed by `FStringStart` 2. `FStringEnd` followed by `String` 3. `FStringEnd` followed by `FStringStart` 4. `String` followed by `String` For f-string tokens, we use the `Indexer` to get the entire range of the f-string. This is the range of the innermost f-string. ## Test Plan Add new test cases for nested f-strings.
dhruvmanila
added a commit
that referenced
this pull request
Sep 28, 2023
## Summary This PR updates the implicit string concatenation rules, specifically `ISC001` and `ISC002` to account for the new f-string tokens. `ISC003` checks for explicit string concatenation and is not affected by PEP 701 because it is based on AST. ### Implementation The implementation is based on the boundary tokens of the f-string which are `FStringStart` and `FStringEnd`. There are 4 cases to look for: 1. `String` followed by `FStringStart` 2. `FStringEnd` followed by `String` 3. `FStringEnd` followed by `FStringStart` 4. `String` followed by `String` For f-string tokens, we use the `Indexer` to get the entire range of the f-string. This is the range of the innermost f-string. ## Test Plan Add new test cases for nested f-strings.
dhruvmanila
added a commit
that referenced
this pull request
Sep 29, 2023
## Summary This PR updates the implicit string concatenation rules, specifically `ISC001` and `ISC002` to account for the new f-string tokens. `ISC003` checks for explicit string concatenation and is not affected by PEP 701 because it is based on AST. ### Implementation The implementation is based on the boundary tokens of the f-string which are `FStringStart` and `FStringEnd`. There are 4 cases to look for: 1. `String` followed by `FStringStart` 2. `FStringEnd` followed by `String` 3. `FStringEnd` followed by `FStringStart` 4. `String` followed by `String` For f-string tokens, we use the `Indexer` to get the entire range of the f-string. This is the range of the innermost f-string. ## Test Plan Add new test cases for nested f-strings.
dhruvmanila
added a commit
that referenced
this pull request
Sep 29, 2023
## Summary This PR updates the implicit string concatenation rules, specifically `ISC001` and `ISC002` to account for the new f-string tokens. `ISC003` checks for explicit string concatenation and is not affected by PEP 701 because it is based on AST. ### Implementation The implementation is based on the boundary tokens of the f-string which are `FStringStart` and `FStringEnd`. There are 4 cases to look for: 1. `String` followed by `FStringStart` 2. `FStringEnd` followed by `String` 3. `FStringEnd` followed by `FStringStart` 4. `String` followed by `String` For f-string tokens, we use the `Indexer` to get the entire range of the f-string. This is the range of the innermost f-string. ## Test Plan Add new test cases for nested f-strings.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This PR updates the implicit string concatenation rules, specifically
ISC001and
ISC002to account for the new f-string tokens.ISC003checks forexplicit string concatenation and is not affected by PEP 701 because it is based
on AST.
Implementation
The implementation is based on the boundary tokens of the f-string which are
FStringStartandFStringEnd. There are 4 cases to look for:Stringfollowed byFStringStartFStringEndfollowed byStringFStringEndfollowed byFStringStartStringfollowed byStringFor f-string tokens, we use the
Indexerto get the entire range of the f-string.This is the range of the innermost f-string.
Test Plan
Add new test cases for nested f-strings.