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

UP032 Failing to Detect format based on Length #10235

Closed
max-muoto opened this issue Mar 5, 2024 · 2 comments · Fixed by #10238
Closed

UP032 Failing to Detect format based on Length #10235

max-muoto opened this issue Mar 5, 2024 · 2 comments · Fixed by #10238
Labels
bug Something isn't working rule Implementing or modifying a lint rule

Comments

@max-muoto
Copy link
Contributor

Running into a weird edge case where UP032 is not picking up the usage of formats.

Take this example, UP032 finds nothing wrong here:

idx = 1
foo = (
    "This is a test, this is a test, this is a test, this is a test {}, {} test".format(
        idx + 1, len([])
    )
)

However, if I reduce the length, it gets flagged:

idx = 1
z = "This is a test, this is a test, {}, {} test".format(
   idx + 1, len([])
)

Ruff Version: 0.3.0
Command: ruff check ..

@MichaReiser
Copy link
Member

Thanks. Agree, that this is unexpected

The rule has a specific check to opt out of providing a fix when the fixed string exceeds the configured line length (because it could introduce new lint errors).

// Avoid refactors that exceed the line length limit.
if !fits_or_shrinks(
&contents,
template.into(),
checker.locator(),
checker.settings.pycodestyle.max_line_length,
checker.settings.tab_size,
) {
return;
}

We do this for a couple of rules but there seems to be no pattern:

  1. Some rules don't provide a fix if the "fixed" string exceeds the configured line width
  2. Some rules don't create a diagnostic when the fixed expression exceeds the configured line width.

I understand 1. but am surprised by 2. Let me see if I can figure out what the reasoning behind it is.

@max-muoto
Copy link
Contributor Author

max-muoto commented Mar 6, 2024

Thanks. Agree, that this is unexpected

The rule has a specific check to opt out of providing a fix when the fixed string exceeds the configured line length (because it could introduce new lint errors).

// Avoid refactors that exceed the line length limit.
if !fits_or_shrinks(
&contents,
template.into(),
checker.locator(),
checker.settings.pycodestyle.max_line_length,
checker.settings.tab_size,
) {
return;
}

We do this for a couple of rules but there seems to be no pattern:

  1. Some rules don't provide a fix if the "fixed" string exceeds the configured line width
  2. Some rules don't create a diagnostic when the fixed expression exceeds the configured line width.

I understand 1. but am surprised by 2. Let me see if I can figure out what the reasoning behind it is.

Thank! It would be nice if there was some way (for formatter users) to still fix it, but then reformat the code to fix the now violating line, but not sure how this would work considering how they're detached. Perhaps this would work better as a setting (i.e. fix violating rules even if it leads to a resulting line greater than the line limit).

MichaReiser added a commit that referenced this issue Mar 6, 2024
…regardless of line-length (`UP032`) (#10238)

## Summary

Fixes #10235

This PR changes `UP032` to flag all `"".format` calls that can
technically be rewritten to an f-string, even if rewritting it to an
fstring, at least automatically, exceeds the line length (or increases
the amount by which it goes over the line length).

I looked at the Git history to understand whether the check prevents
some false positives (reported by an issue), but i haven't found a
compelling reason to limit the rule to only flag format calls that stay
in the line length limit:

* #7818 Changed the heuristic to
determine if the fix fits to address
#7810
* #1905 first version of the rule 


I did take a look at pyupgrade and couldn't find a similar check, at
least not in the rule code (maybe it's checked somewhere else?)
https://github.com/asottile/pyupgrade/blob/main/pyupgrade/_plugins/fstrings.py


## Breaking Change?

This could be seen as a breaking change according to ruff's [versioning
policy](https://docs.astral.sh/ruff/versioning/):

> The behavior of a stable rule is changed
  
  * The scope of a stable rule is significantly increased
  * The intent of the rule changes
* Does not include bug fixes that follow the original intent of the rule

It does increase the scope of the rule, but it is in the original intent
of the rule (so it's not).

## Test Plan

See changed test output
nkxxll pushed a commit to nkxxll/ruff that referenced this issue Mar 10, 2024
…regardless of line-length (`UP032`) (astral-sh#10238)

## Summary

Fixes astral-sh#10235

This PR changes `UP032` to flag all `"".format` calls that can
technically be rewritten to an f-string, even if rewritting it to an
fstring, at least automatically, exceeds the line length (or increases
the amount by which it goes over the line length).

I looked at the Git history to understand whether the check prevents
some false positives (reported by an issue), but i haven't found a
compelling reason to limit the rule to only flag format calls that stay
in the line length limit:

* astral-sh#7818 Changed the heuristic to
determine if the fix fits to address
astral-sh#7810
* astral-sh#1905 first version of the rule 


I did take a look at pyupgrade and couldn't find a similar check, at
least not in the rule code (maybe it's checked somewhere else?)
https://github.com/asottile/pyupgrade/blob/main/pyupgrade/_plugins/fstrings.py


## Breaking Change?

This could be seen as a breaking change according to ruff's [versioning
policy](https://docs.astral.sh/ruff/versioning/):

> The behavior of a stable rule is changed
  
  * The scope of a stable rule is significantly increased
  * The intent of the rule changes
* Does not include bug fixes that follow the original intent of the rule

It does increase the scope of the rule, but it is in the original intent
of the rule (so it's not).

## Test Plan

See changed test output
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working rule Implementing or modifying a lint rule
Projects
None yet
2 participants