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

[ruff] Recognize more expressions (RUF027) #15247

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

InSyncWithFoo
Copy link
Contributor

Summary

Resolves #15227.

Test Plan

cargo nextest run and cargo insta test.

Copy link
Contributor

github-actions bot commented Jan 4, 2025

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

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

apache/airflow (+3 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview --select ALL

+ dev/breeze/src/airflow_breeze/utils/kubernetes_utils.py:327:13: RUF027 Possible f-string without an `f` prefix
+ providers/src/airflow/providers/apache/spark/hooks/spark_connect.py:76:30: RUF027 Possible f-string without an `f` prefix
+ scripts/ci/pre_commit/check_min_python_version.py:38:17: RUF027 Possible f-string without an `f` prefix

ibis-project/ibis (+1 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview

+ ibis/backends/__init__.py:919:33: RUF027 Possible f-string without an `f` prefix

latchbio/latch (+4 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview

+ src/latch_cli/nextflow/config.py:148:35: RUF027 Possible f-string without an `f` prefix
+ src/latch_cli/snakemake/config/parser.py:116:20: RUF027 Possible f-string without an `f` prefix
+ src/latch_cli/snakemake/config/parser.py:248:13: RUF027 Possible f-string without an `f` prefix
+ src/latch_cli/snakemake/serialize_utils.py:179:13: RUF027 Possible f-string without an `f` prefix

lnbits/lnbits (+1 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview

+ lnbits/core/helpers.py:85:30: RUF027 Possible f-string without an `f` prefix

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

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview

+ pandas/core/generic.py:12613:22: RUF027 Possible f-string without an `f` prefix

rotki/rotki (+6 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview

+ rotkehlchen/tests/api/blockchain/test_base.py:949:33: RUF027 Possible f-string without an `f` prefix
+ rotkehlchen/tests/exchanges/test_bitfinex.py:70:22: RUF027 Possible f-string without an `f` prefix
+ rotkehlchen/tests/exchanges/test_bitfinex.py:80:22: RUF027 Possible f-string without an `f` prefix
+ rotkehlchen/tests/exchanges/test_bitfinex.py:89:22: RUF027 Possible f-string without an `f` prefix
+ rotkehlchen/tests/utils/checks.py:15:33: RUF027 Possible f-string without an `f` prefix
+ rotkehlchen/tests/utils/checks.py:16:33: RUF027 Possible f-string without an `f` prefix

zulip/zulip (+4 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview --select ALL

+ corporate/tests/test_stripe.py:1650:46: RUF027 Possible f-string without an `f` prefix
+ zerver/lib/event_schema.py:457:30: RUF027 Possible f-string without an `f` prefix
+ zerver/management/commands/export_search.py:129:17: RUF027 Possible f-string without an `f` prefix
+ zerver/tests/test_signup.py:3140:13: RUF027 Possible f-string without an `f` prefix

pytest-dev/pytest (+2 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview

+ testing/test_pathlib.py:1365:13: RUF027 Possible f-string without an `f` prefix
+ testing/test_terminal.py:1103:13: RUF027 Possible f-string without an `f` prefix

astropy/astropy (+7 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview

+ astropy/coordinates/representation/geodetic.py:18:21: RUF027 Possible f-string without an `f` prefix
+ astropy/coordinates/transformations/affine.py:68:17: RUF027 Possible f-string without an `f` prefix
+ astropy/table/table.py:98:16: RUF027 Possible f-string without an `f` prefix
+ astropy/uncertainty/core.py:579:17: RUF027 Possible f-string without an `f` prefix
+ astropy/utils/tests/test_decorators.py:725:18: RUF027 Possible f-string without an `f` prefix
+ astropy/utils/tests/test_decorators.py:821:17: RUF027 Possible f-string without an `f` prefix
+ astropy/wcs/utils.py:253:17: RUF027 Possible f-string without an `f` prefix

Changes by rule (1 rules affected)

code total + violation - violation + fix - fix
RUF027 29 29 0 0 0

@InSyncWithFoo
Copy link
Contributor Author

The true-positive-turned-false-negative:

raise ValueError(
    "Scale {scale!r} is not in the allowed scales {sorted(self.SCALES)}"
)

#9849 has a bit more context on why strings whose placeholders' names are the same as that of built-ins are ignored. This interacted poorly with the new logic; in addition, it fails to recognize dunder names:

print('{__path__}')  # Currently a false negative

@InSyncWithFoo
Copy link
Contributor Author

The new changes introduced a few true positives, most notably:

@latchbio/latch:

params_path.write_text(dedent(r"""
        ...

        generated_parameters = {
        __params__
        }

        """).replace("__params__", reindent("".join(params), 1)))

@astropy/astropy:

geodetic_base_doc = """{__doc__}
    ...
"""


@format_doc(geodetic_base_doc)
class BaseGeodeticRepresentation(BaseRepresentation): ...

The former is embedded code, while the latter is dynamically interpolated. These cannot be detected reliably in the first place, so I think the false positives are acceptable.

@AlexWaygood
Copy link
Member

I haven't looked at this PR yet, but note that RUF027 already has too many false positives for us to consider stabilising it. I definitely don't think we should be making changes that introduce new false positives here, honestly

@InSyncWithFoo
Copy link
Contributor Author

If so, this PR might as well be closed, as this is now considered an error:

@apache/airflow:

python_major_minor_version = run_command(
    [
        str(PYTHON_BIN_PATH),
        "-c",
        "import sys; print(f'{sys.version_info.major}.{sys.version_info.minor}')",
    ],
).stdout.strip()

@MichaReiser
Copy link
Member

It does find some new actual violations but we should look into if we can find a way to reduce the false positives.

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