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

ISC003 gives false positive when two different string types in one line are mandatory #6595

Closed
mmarras opened this issue Aug 15, 2023 · 7 comments

Comments

@mmarras
Copy link

mmarras commented Aug 15, 2023

When defining hovertemplates they use a magic string (e.g. %{x} to denote the x-data) that cannot be inside an fstring. If one wants to combine the former with an fstring on the same line it flags the thing and it is impossible to comply to ruff ISC without breaking the code. So even if one decides to keep the rule for same line, it should make exception for mixed string types.

Correct :

import plotly.graph_objs as go
string = "Data1"
go.Figure(data=go.Scatter(
    x=[1.123123,2.12321,3.12321],
    y=[1.12321,2.12321,3.12321],
    hovertemplate="%{x:.2f}<br>%{y:.3f}" + f"<extra>{string}</extra>")
          )

The above codeblock but with hovertemplate=f"%{x:.2f}<br>%{y:.3f}<extra>{string}</extra>") fails.

NameError: name 'x' is not defined

Of course the correct version (see codeblock) triggers

Explicitly concatenated string should be implicitly concatenatedRuffISC003

or when implicitly concatenated:

Implicitly concatenated string literals on one lineRuffISC001

Originally posted by @mmarras in #5332 (comment)

@charliermarsh
Copy link
Member

Having trouble reproducing.

Given:

import plotly.graph_objs as go
string = "Data1"
go.Figure(data=go.Scatter(
    x=[1.123123,2.12321,3.12321],
    y=[1.12321,2.12321,3.12321],
    hovertemplate="%{x:.2f}<br>%{y:.3f}" + f"<extra>{string}</extra>")
          )

Running ruff check foo.py --select ISC --isolated -n doesn't yield any violations.

Was it broken over multiple lines? Like:

import plotly.graph_objs as go
string = "Data1"
go.Figure(data=go.Scatter(
    x=[1.123123,2.12321,3.12321],
    y=[1.12321,2.12321,3.12321],
    hovertemplate=(
        "%{x:.2f}<br>%{y:.3f}" +
        f"<extra>{string}</extra>")
    )
)

@mmarras
Copy link
Author

mmarras commented Aug 15, 2023

Indeed, this only shows in my VScode (1.81.0) extension (v2023.32.0, ruff 0.0.277, I assume), not if I run ruff cli with ruff (0.0.284). My bad.

image

@zanieb
Copy link
Member

zanieb commented Aug 15, 2023

Hm I think the VSCode extension should use your externally installed Ruff if available (so an update is not needed to get the latest version). Anyway, it sounds like we've resolved this on the latest version. Let us know if there's more we can do!

@zanieb zanieb closed this as completed Aug 15, 2023
@charliermarsh
Copy link
Member

I think it's not quite resolved because the request is that we don't flag explicit concatenations with mixed-kind strings, like:

import plotly.graph_objs as go
string = "Data1"
go.Figure(data=go.Scatter(
    x=[1.123123,2.12321,3.12321],
    y=[1.12321,2.12321,3.12321],
    hovertemplate=(
        "%{x:.2f}<br>%{y:.3f}" +
        f"<extra>{string}</extra>")
    )
)

@charliermarsh
Copy link
Member

(The confusion above is about whether it flags for multiline strings or single-line strings, related to the repro.)

@charliermarsh charliermarsh reopened this Aug 15, 2023
@mmarras
Copy link
Author

mmarras commented Aug 15, 2023

Hm I think the VSCode extension should use your externally installed Ruff if available (so an update is not needed to get the latest version). Anyway, it sounds like we've resolved this on the latest version. Let us know if there's more we can do!

So, VScode was in a different env than my terminal conda env, therefore the missmatch in ruff results editor vs cli.

As far as I am concerned the issue is solved (I checked, as of 0.0.281). Conclusion stands: my bad.

For me it was always about the single line example. I was happy as soon as an explicit concat of a mixed-kind one-line string wasn't flagged anymore. But I'm using plotly quite a bit so, if it comes up in another constellation, I can for sure report back.

@charliermarsh
Copy link
Member

Okay, sounds good! I'm glad it's resolved for this use-case, thank you for reporting back :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants