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

Update F541 to use new f-string tokens #7327

Merged
merged 3 commits into from
Sep 18, 2023
Merged

Conversation

dhruvmanila
Copy link
Member

@dhruvmanila dhruvmanila commented Sep 13, 2023

Summary

This PR updates the F541 rule to use the new f-string tokens.

Test Plan

Add new test case and uncomment a broken test case.

fixes: #7292

@dhruvmanila dhruvmanila linked an issue Sep 13, 2023 that may be closed by this pull request
@dhruvmanila dhruvmanila added python312 Related to Python 3.12 rule Implementing or modifying a lint rule labels Sep 13, 2023
@dhruvmanila dhruvmanila changed the base branch from dhruv/issue-7291 to dhruv/pep-701 September 14, 2023 02:47
@dhruvmanila dhruvmanila marked this pull request as ready for review September 14, 2023 03:34
let first_char = locator.slice(TextRange::at(range.start(), TextSize::from(1)));
.filter_map(move |(tok, range)| match tok {
Tok::FStringStart => {
current_f_string_start = range.start();
Copy link
Member

Choose a reason for hiding this comment

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

How does this work with multiple nested fstrings?

f"{f"{a + f"{b}"}"}"

I would expect that f"{b}" overrides the start position of f"{a + f"{b}"}"

Copy link
Member Author

@dhruvmanila dhruvmanila Sep 15, 2023

Choose a reason for hiding this comment

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

To give some context, the autofix for the rule F541 is to remove the f prefix from the f-string without any placeholders. The placeholder checks are done at an expression level by checking if there's any FormattedValue. The function you're referring to is to get the f prefix range and the f-string range. At this point there can't be any nested f-strings.

Also, the reason the function returns an iterator is because of implicitly concatenated strings.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the function name is a bit confusing which I'll rename. I've also added docs to the function.

Copy link
Member

@MichaReiser MichaReiser Sep 15, 2023

Choose a reason for hiding this comment

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

Hmm, but the documentation mentions that this function returns all nested f-string ranges, meaning there need to be placeholders? It's also unclear to me where the filtering out of f-strings without placeholders happens.

So I think we need to update the documentation to match its actual behavior (or it's too early in the morning and I shouldn't be reviewing PR's 😅 )

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, but the documentation mentions that this function returns all nested f-string ranges, meaning there need to be placeholders?

I've updated the documentation but I'm curious as to where was "nested f-string ranges" mentioned? 😅

Comment on lines 110 to 111
if !values
.iter()
.any(|value| matches!(value, Expr::FormattedValue(_)))
{
for (prefix_range, tok_range) in
Copy link
Member Author

Choose a reason for hiding this comment

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

It's also unclear to me where the filtering out of f-strings without placeholders happens.

@MichaReiser

Copy link
Member

Choose a reason for hiding this comment

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

Thanks. The issue I see is that find_useless_f_strings returns all fstrings but with potentially incorrect ranges. So naming that function find_useless_f_strings is misleading in my view because not all fstrings returned by that function are useless.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I totally agree. I've renamed it to fstring_prefix_and_tok_range but the main reason it returns an iterator is because the f-string can contain multiple implicitly concatenated (f-)strings.

because not all fstrings returned by that function are useless.

Can you expand on this? For example, in the below code snippet it would return (prefix range, token range) for the "first" and the "last" string and not for the "normal" string. Both f-strings are useless in the sense that there are no placeholders used and so the f prefix is useless. I don't think it ever returns incorrect ranges.

  f"first" "normal" rf"second"
# ^                  ^             (prefix range)
# ^^^^^^^^          ^^^^^^^^^^     (token range)

# (0..1, 0..8)      - `f"first"`
# (19..20, 18..28)  - `rf"second"`

@dhruvmanila dhruvmanila merged commit 641c4a7 into dhruv/pep-701 Sep 18, 2023
10 of 13 checks passed
@dhruvmanila dhruvmanila deleted the dhruv/issue-7292 branch September 18, 2023 16:44
dhruvmanila added a commit that referenced this pull request Sep 19, 2023
## Summary

This PR updates the `F541` rule to use the new f-string tokens.

## Test Plan

Add new test case and uncomment a broken test case.

fixes: #7292
dhruvmanila added a commit that referenced this pull request Sep 20, 2023
## Summary

This PR updates the `F541` rule to use the new f-string tokens.

## Test Plan

Add new test case and uncomment a broken test case.

fixes: #7292
dhruvmanila added a commit that referenced this pull request Sep 21, 2023
## Summary

This PR updates the `F541` rule to use the new f-string tokens.

## Test Plan

Add new test case and uncomment a broken test case.

fixes: #7292
dhruvmanila added a commit that referenced this pull request Sep 22, 2023
## Summary

This PR updates the `F541` rule to use the new f-string tokens.

## Test Plan

Add new test case and uncomment a broken test case.

fixes: #7292
dhruvmanila added a commit that referenced this pull request Sep 22, 2023
## Summary

This PR updates the `F541` rule to use the new f-string tokens.

## Test Plan

Add new test case and uncomment a broken test case.

fixes: #7292
dhruvmanila added a commit that referenced this pull request Sep 22, 2023
## Summary

This PR updates the `F541` rule to use the new f-string tokens.

## Test Plan

Add new test case and uncomment a broken test case.

fixes: #7292
dhruvmanila added a commit that referenced this pull request Sep 26, 2023
## Summary

This PR updates the `F541` rule to use the new f-string tokens.

## Test Plan

Add new test case and uncomment a broken test case.

fixes: #7292
dhruvmanila added a commit that referenced this pull request Sep 27, 2023
## Summary

This PR updates the `F541` rule to use the new f-string tokens.

## Test Plan

Add new test case and uncomment a broken test case.

fixes: #7292
dhruvmanila added a commit that referenced this pull request Sep 28, 2023
## Summary

This PR updates the `F541` rule to use the new f-string tokens.

## Test Plan

Add new test case and uncomment a broken test case.

fixes: #7292
dhruvmanila added a commit that referenced this pull request Sep 29, 2023
## Summary

This PR updates the `F541` rule to use the new f-string tokens.

## Test Plan

Add new test case and uncomment a broken test case.

fixes: #7292
dhruvmanila added a commit that referenced this pull request Sep 29, 2023
## Summary

This PR updates the `F541` rule to use the new f-string tokens.

## Test Plan

Add new test case and uncomment a broken test case.

fixes: #7292
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
python312 Related to Python 3.12 rule Implementing or modifying a lint rule
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update F541 to use new tokens to detect f-strings
2 participants