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

allow human-friendly rule names in noqa directives #11757

Closed
wants to merge 3 commits into from

Conversation

Humandoodlebug
Copy link

Summary

Works towards #1773.

Allows users to write noqa directives with human-friendly rule names. Users can write noqa directives with a comma-separated list of rule names (# noqa: ambiguous-variable-name , unused-variable), or even a mix of codes and names (# noqa: E741, unused-variable).

Additionally, rules have been added for enforcing the exclusive use of either codes or names, with fixes defined for both.

This is a changeset we are looking to use internally at Pexip. We have been migrating from pylint to ruff, but had pushback due to codes being required for noqa directives (pylint allows human-friendly rule names here). We'd like to upstream these changes, but are aware you may wish to implement some or all of this differently, so we're submitting this as a draft PR for feedback.

Caveats

We made some compromises with the parsing rules for noqa directives that reference rules by name to avoid confusing comments with rule names. The most significant one is that lists of rule names must be comma separated, which allows us to determine if the next word is supposed to be a rule name or is just a comment. This stricter lexing rule does not apply to lists of rule codes. (so # noqa: E741 F841 is still fine).

We also require a rule name to be followed by either a comma or a whitespace character (or the end of the line). This prevents us from seeing e.g. some_rule_name and interpreting it as a rule named some followed by a comment _rule_name.

Test Plan

  • Updated existing noqa snapshot tests and added new ones to cover using rule names.
  • Added snapshot tests for new RUF102 (noqa-by-code) and RUF103 (noqa-by-name) lints.

@Humandoodlebug Humandoodlebug force-pushed the human-friendly-names branch 2 times, most recently from c4ee601 to ae61646 Compare June 5, 2024 17:22
@MichaelOultram-pexip
Copy link
Contributor

We've triaged the ecosystem test failure. The job is timing out in CI, but we believe this is due to the python script and not a regression in ruff performance.

The ecosystem python script will produce the diff between baseline ruff and this PR's ruff, and we believe it's this diff which is taking so much time. The diff will be extremely large as the rule name will now be printed everywhere the rule code is printed. The ruff executable runs in the same amount of time from our investigation.

MichaelOultram-pexip pushed a commit to pexip/ruff that referenced this pull request Jun 5, 2024
@MichaelOultram-pexip MichaelOultram-pexip force-pushed the human-friendly-names branch 3 times, most recently from a2558e9 to 95e97d9 Compare July 12, 2024 16:21
MichaelOultram-pexip added a commit to pexip/ruff that referenced this pull request Jul 12, 2024
MichaelOultram-pexip added a commit to pexip/ruff that referenced this pull request Aug 19, 2024
@sbrugman sbrugman mentioned this pull request Nov 20, 2024
1 task
@kaddkaka
Copy link

Does this PR support multiple names per rule as suggested in #1773 (comment) ?

@MichaelOultram-pexip
Copy link
Contributor

MichaelOultram-pexip commented Nov 20, 2024

@kaddkaka the PR in it's current state does not have this functionality. I will look into extending crates/ruff_linter/src/rule_redirects.rs to support rule names. This will take me a few days due to other commitments.

Copy link
Contributor

github-actions bot commented Nov 20, 2024

ruff-ecosystem results

Linter (stable)

ℹ️ ecosystem check detected linter changes. (+3 -0 violations, +0 -0 fixes in 1 projects; 53 projects unchanged)

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

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

+ tests/decorators/test_setup_teardown.py:129:34: RUF100 [*] Unused `noqa` directive (unknown: `check`)
+ tests/decorators/test_setup_teardown.py:147:34: RUF100 [*] Unused `noqa` directive (unknown: `check`)
+ tests/decorators/test_setup_teardown.py:992:30: RUF100 [*] Unused `noqa` directive (unknown: `check`)

Changes by rule (1 rules affected)

code total + violation - violation + fix - fix
RUF100 3 3 0 0 0

Linter (preview)

ℹ️ ecosystem check detected linter changes. (+9665 -338 violations, +0 -0 fixes in 20 projects; 34 projects unchanged)

DisnakeDev/disnake (+36 -0 violations, +0 -0 fixes)

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

+ disnake/enums.py:839:26: RUF102 [*] Noqa directive lists rule codes instead of rule names: RUF001 (ambiguous-unicode-character-string)
+ disnake/ext/commands/bot_base.py:122:83: RUF102 [*] Noqa directive lists rule codes instead of rule names: TRY004 (type-check-without-type-error)
+ disnake/ext/commands/core.py:512:25: RUF102 [*] Noqa directive lists rule codes instead of rule names: B012 (jump-statement-in-finally)
+ disnake/ext/commands/ctx_menus_core.py:121:25: RUF102 [*] Noqa directive lists rule codes instead of rule names: B012 (jump-statement-in-finally)
+ disnake/ext/commands/ctx_menus_core.py:221:25: RUF102 [*] Noqa directive lists rule codes instead of rule names: B012 (jump-statement-in-finally)
+ disnake/ext/commands/slash_core.py:654:25: RUF102 [*] Noqa directive lists rule codes instead of rule names: B012 (jump-statement-in-finally)
+ disnake/ext/commands/view.py:18:16: RUF102 [*] Noqa directive lists rule codes instead of rule names: RUF001 (ambiguous-unicode-character-string)
+ disnake/ext/commands/view.py:21:16: RUF102 [*] Noqa directive lists rule codes instead of rule names: RUF001 (ambiguous-unicode-character-string)
+ disnake/ext/commands/view.py:8:16: RUF102 [*] Noqa directive lists rule codes instead of rule names: RUF001 (ambiguous-unicode-character-string)
+ disnake/ext/commands/view.py:9:16: RUF102 [*] Noqa directive lists rule codes instead of rule names: RUF001 (ambiguous-unicode-character-string)
... 26 additional changes omitted for project

RasaHQ/rasa (+103 -0 violations, +0 -0 fixes)

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

+ .github/tests/test_download_pretrained.py:102:19: RUF102 [*] Noqa directive lists rule codes instead of rule names: RUF015 (unnecessary-iterable-allocation-for-first-element)
+ .github/tests/test_download_pretrained.py:10:29: RUF102 [*] Noqa directive lists rule codes instead of rule names: E402 (module-import-not-at-top-of-file)
+ .github/tests/test_download_pretrained.py:26:19: RUF102 [*] Noqa directive lists rule codes instead of rule names: RUF015 (unnecessary-iterable-allocation-for-first-element)
+ .github/tests/test_download_pretrained.py:46:19: RUF102 [*] Noqa directive lists rule codes instead of rule names: RUF015 (unnecessary-iterable-allocation-for-first-element)
+ .github/tests/test_download_pretrained.py:63:19: RUF102 [*] Noqa directive lists rule codes instead of rule names: RUF015 (unnecessary-iterable-allocation-for-first-element)
+ .github/tests/test_download_pretrained.py:83:19: RUF102 [*] Noqa directive lists rule codes instead of rule names: RUF015 (unnecessary-iterable-allocation-for-first-element)
+ .github/tests/test_mr_generate_summary.py:4:49: RUF102 [*] Noqa directive lists rule codes instead of rule names: E402 (module-import-not-at-top-of-file)
+ .github/tests/test_mr_publish_results.py:7:35: RUF102 [*] Noqa directive lists rule codes instead of rule names: E402 (module-import-not-at-top-of-file)
+ .github/tests/test_validate_gpus.py:8:22: RUF102 [*] Noqa directive lists rule codes instead of rule names: E402 (module-import-not-at-top-of-file)
+ .github/tests/test_validate_gpus.py:9:23: RUF102 [*] Noqa directive lists rule codes instead of rule names: E402 (module-import-not-at-top-of-file)
... 93 additional changes omitted for project

apache/airflow (+512 -15 violations, +0 -0 fixes)

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

+ airflow/cli/commands/dag_processor_command.py:42:81: RUF102 [*] Noqa directive lists rule codes instead of rule names: F401 (unused-import)
+ airflow/cli/commands/dag_processor_command.py:43:53: RUF102 [*] Noqa directive lists rule codes instead of rule names: F401 (unused-import)
+ airflow/cli/commands/task_command.py:326:81: RUF102 [*] Noqa directive lists rule codes instead of rule names: F401 (unused-import)
+ airflow/cli/commands/task_command.py:327:53: RUF102 [*] Noqa directive lists rule codes instead of rule names: F401 (unused-import)
+ airflow/configuration.py:1:1: D100 Missing docstring in public module
- airflow/configuration.py:1:1: D100 Missing docstring in public module
+ airflow/configuration.py:303:65: RUF102 [*] Noqa directive lists rule codes instead of rule names: UP006 (non-pep585-annotation)
+ airflow/macros/__init__.py:20:14: RUF102 [*] Noqa directive lists rule codes instead of rule names: F401 (unused-import)
+ airflow/macros/__init__.py:21:14: RUF102 [*] Noqa directive lists rule codes instead of rule names: F401 (unused-import)
... 56 additional changes omitted for rule F401
+ airflow/models/baseoperator.py:119:49: RUF102 [*] Noqa directive lists rule codes instead of rule names: TC001 (typing-only-first-party-import)
... 517 additional changes omitted for project

apache/superset (+1696 -170 violations, +0 -0 fixes)

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

+ scripts/build_docker.py:143:15: RUF102 [*] Noqa directive lists rule codes instead of rule names: F841 (unused-variable)
+ scripts/build_docker.py:287:35: RUF102 [*] Noqa directive lists rule codes instead of rule names: F841 (unused-variable)
+ scripts/build_docker.py:85:44: RUF102 [*] Noqa directive lists rule codes instead of rule names: E741 (ambiguous-variable-name)
+ superset/__init__.py:20:38: RUF102 [*] Noqa directive lists rule codes instead of rule names: F401 (unused-import)
+ superset/__init__.py:22:18: RUF102 [*] Noqa directive lists rule codes instead of rule names: F401 (unused-import)
+ superset/__init__.py:24:10: RUF102 [*] Noqa directive lists rule codes instead of rule names: F401 (unused-import)
+ superset/__init__.py:25:20: RUF102 [*] Noqa directive lists rule codes instead of rule names: F401 (unused-import)
+ superset/__init__.py:29:24: RUF102 [*] Noqa directive lists rule codes instead of rule names: F401 (unused-import)
+ superset/__init__.py:30:16: RUF102 [*] Noqa directive lists rule codes instead of rule names: F401 (unused-import)
... 399 additional changes omitted for rule F401
+ superset/commands/importers/v1/__init__.py:82:34: RUF102 [*] Noqa directive lists rule codes instead of rule names: F811 (redefined-while-unused)
... 1856 additional changes omitted for project

bokeh/bokeh (+17 -1 violations, +0 -0 fixes)

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

+ examples/basic/data/color_mappers.py:9:5: RUF102 [*] Noqa directive lists rule codes instead of rule names: E501 (line-too-long)
+ examples/interaction/js_callbacks/color_sliders.py:9:5: RUF102 [*] Noqa directive lists rule codes instead of rule names: E501 (line-too-long)
+ examples/models/buttons.py:15:5: RUF102 [*] Noqa directive lists rule codes instead of rule names: E501 (line-too-long)
+ examples/models/calendars.py:14:5: RUF102 [*] Noqa directive lists rule codes instead of rule names: E501 (line-too-long)
+ examples/models/sliders.py:9:5: RUF102 [*] Noqa directive lists rule codes instead of rule names: E501 (line-too-long)
+ examples/models/widgets.py:11:5: RUF102 [*] Noqa directive lists rule codes instead of rule names: E501 (line-too-long)
... 2 additional changes omitted for rule E501
+ src/bokeh/core/property/pd.py:31:35: RUF102 [*] Noqa directive lists rule codes instead of rule names: F401 (unused-import)
+ src/bokeh/core/property/pd.py:32:54: RUF102 [*] Noqa directive lists rule codes instead of rule names: F401 (unused-import)
+ src/bokeh/model/util.py:167:28: RUF102 [*] Noqa directive lists rule codes instead of rule names: F401 (unused-import)
+ src/bokeh/model/util.py:168:30: RUF102 [*] Noqa directive lists rule codes instead of rule names: F401 (unused-import)
... 8 additional changes omitted for project

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

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

+ .github/workflows/algolia/configure-algolia-api.py:1:37: RUF102 [*] Noqa directive lists rule codes instead of rule names: INP001 (implicit-namespace-package)
+ .github/workflows/algolia/upload-algolia-api.py:164:54: RUF102 [*] Noqa directive lists rule codes instead of rule names: T201 (print)
+ .github/workflows/algolia/upload-algolia-api.py:178:56: RUF102 [*] Noqa directive lists rule codes instead of rule names: T201 (print)
+ .github/workflows/algolia/upload-algolia-api.py:190:66: RUF102 [*] Noqa directive lists rule codes instead of rule names: T201 (print)
+ .github/workflows/algolia/upload-algolia-api.py:1:37: RUF102 [*] Noqa directive lists rule codes instead of rule names: INP001 (implicit-namespace-package)
+ .github/workflows/algolia/upload-algolia-api.py:203:53: RUF102 [*] Noqa directive lists rule codes instead of rule names: T201 (print)
+ .github/workflows/algolia/upload-algolia-api.py:208:66: RUF102 [*] Noqa directive lists rule codes instead of rule names: T201 (print)
+ .github/workflows/algolia/upload-algolia.py:1:37: RUF102 [*] Noqa directive lists rule codes instead of rule names: INP001 (implicit-namespace-package)
+ ci/make_geography_db.py:98:21: RUF102 [*] Noqa directive lists rule codes instead of rule names: T201 (print)
... 16 additional changes omitted for rule T201
+ ibis/__init__.py:140:24: RUF102 [*] Noqa directive lists rule codes instead of rule names: F405 (undefined-local-with-import-star-usage)
... 241 additional changes omitted for project

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

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

+ lnbits/bolt11.py:2:25: RUF102 [*] Noqa directive lists rule codes instead of rule names: F401 (unused-import)
+ lnbits/bolt11.py:5:14: RUF102 [*] Noqa directive lists rule codes instead of rule names: F401 (unused-import)
+ lnbits/bolt11.py:6:14: RUF102 [*] Noqa directive lists rule codes instead of rule names: F401 (unused-import)
+ lnbits/core/views/api.py:44:52: RUF102 [*] Noqa directive lists rule codes instead of rule names: F401 (unused-import)
+ lnbits/core/views/public_api.py:54:51: RUF102 [*] Noqa directive lists rule codes instead of rule names: RUF006 (asyncio-dangling-task)
+ lnbits/nodes/lndrest.py:179:65: RUF102 [*] Noqa directive lists rule codes instead of rule names: RUF006 (asyncio-dangling-task)
+ lnbits/wallets/boltz.py:75:112: RUF102 [*] Noqa directive lists rule codes instead of rule names: E501 (line-too-long)
+ tests/wallets/test_blink.py:135:92: RUF102 [*] Noqa directive lists rule codes instead of rule names: E501 (line-too-long)
+ tests/wallets/test_blink.py:46:317: RUF102 [*] Noqa directive lists rule codes instead of rule names: E501 (line-too-long)
+ tests/wallets/test_blink.py:97:100: RUF102 [*] Noqa directive lists rule codes instead of rule names: E501 (line-too-long)
... 2 additional changes omitted for project

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

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

- asv_bench/benchmarks/pandas_vb_common.py:18:31: F401 `pandas._testing` imported but unused; consider using `importlib.util.find_spec` to test for availability
- pandas/api/types/__init__.py:7:1: F403 `from pandas.core.dtypes.api import *` used; unable to detect undefined names
- pandas/conftest.py:1969:89: E501 Line too long (156 > 88)
- pandas/core/api.py:44:38: ICN001 `pandas.core.construction.array` should be imported as `pd_array`
- pandas/core/dtypes/cast.py:1257:89: E501 Line too long (104 > 88)
- pandas/core/dtypes/cast.py:1262:89: E501 Line too long (106 > 88)
- pandas/core/groupby/groupby.py:5300:89: E501 Line too long (106 > 88)
- pandas/io/common.py:284:12: TID251 `urllib.request.urlopen` is banned: Use pandas.io.common.urlopen instead of urllib.request.urlopen
- pandas/io/formats/html.py:98:30: RUF003 Comment contains ambiguous `×` (MULTIPLICATION SIGN). Did you mean `x` (LATIN SMALL LETTER X)?
- pandas/tests/copy_view/index/test_index.py:78:5: F841 Local variable `idx` is assigned to but never used
... 78 additional changes omitted for project

... Truncated remaining completed project reports due to GitHub comment length restrictions

Changes by rule (151 rules affected)

code total + violation - violation + fix - fix
E501 6666 6627 39 0 0
E402 1084 1084 0 0 0
F401 563 561 2 0 0
ANN201 238 119 119 0 0
F811 124 110 14 0 0
F405 124 124 0 0 0
F403 113 112 1 0 0
D100 86 43 43 0 0
F841 78 75 3 0 0
F541 58 58 0 0 0
E711 44 38 6 0 0
RUF001 39 9 30 0 0
PLR6301 38 19 19 0 0
TC001 37 37 0 0 0
E712 28 27 1 0 0
T201 28 28 0 0 0
PT009 26 26 0 0 0
F821 22 22 0 0 0
S610 22 22 0 0 0
C901 20 19 1 0 0
B015 19 19 0 0 0
D102 18 9 9 0 0
B018 18 16 2 0 0
UP031 17 17 0 0 0
UP032 17 17 0 0 0
F822 17 0 17 0 0
RUF012 15 15 0 0 0
N802 15 15 0 0 0
S608 14 14 0 0 0
RUF018 13 13 0 0 0
PLW2901 13 13 0 0 0
RUF015 12 12 0 0 0
ANN001 12 6 6 0 0
E741 11 11 0 0 0
TID251 10 7 3 0 0
D400 10 6 4 0 0
SIM115 10 10 0 0 0
UP006 9 9 0 0 0
PT012 8 8 0 0 0
BLE001 8 8 0 0 0
N801 8 8 0 0 0
PERF401 8 8 0 0 0
S102 7 7 0 0 0
TC003 7 7 0 0 0
ARG002 7 7 0 0 0
B012 6 6 0 0 0
PLW0603 6 6 0 0 0
B007 6 6 0 0 0
CPY001 6 3 3 0 0
E731 6 6 0 0 0
E722 6 6 0 0 0
PLC1901 6 6 0 0 0
N815 6 6 0 0 0
E241 6 6 0 0 0
S307 5 5 0 0 0
DTZ005 5 5 0 0 0
UP007 5 5 0 0 0
S105 5 5 0 0 0
S404 5 5 0 0 0
PT017 5 5 0 0 0
PLE0704 5 5 0 0 0
B023 4 4 0 0 0
E721 4 4 0 0 0
D212 4 2 2 0 0
F722 4 4 0 0 0
INP001 4 4 0 0 0
SIM201 4 4 0 0 0
B008 4 4 0 0 0
ARG001 4 4 0 0 0
PLW1641 4 4 0 0 0
S301 4 4 0 0 0
N805 4 4 0 0 0
N806 4 4 0 0 0
TRY002 3 3 0 0 0
S101 3 3 0 0 0
TC004 3 2 1 0 0
D205 3 2 1 0 0
RUF100 3 3 0 0 0
F402 3 3 0 0 0
S310 3 3 0 0 0
B027 3 3 0 0 0
B017 3 3 0 0 0
UP036 3 3 0 0 0
S403 3 3 0 0 0
PLR1714 3 3 0 0 0
D101 2 2 0 0 0
ANN204 2 1 1 0 0
D104 2 1 1 0 0
D202 2 1 1 0 0
D415 2 1 1 0 0
PLR0912 2 1 1 0 0
D200 2 1 1 0 0
E713 2 2 0 0 0
ICN001 2 1 1 0 0
RET503 2 2 0 0 0
S106 2 2 0 0 0
S603 2 2 0 0 0
SIM202 2 2 0 0 0
RUF006 2 2 0 0 0
S108 2 2 0 0 0
B024 2 2 0 0 0
S308 2 2 0 0 0
B006 2 2 0 0 0
SIM118 2 2 0 0 0
FURB118 2 2 0 0 0
N811 2 2 0 0 0
S501 2 2 0 0 0
RUF003 2 0 2 0 0
TRY004 1 1 0 0 0
PLC0205 1 1 0 0 0
B035 1 1 0 0 0
I001 1 1 0 0 0
PGH005 1 1 0 0 0
PLE0604 1 1 0 0 0
RUF017 1 1 0 0 0
T203 1 1 0 0 0
UP035 1 1 0 0 0
B009 1 1 0 0 0
RUF002 1 1 0 0 0
D417 1 1 0 0 0
RUF027 1 1 0 0 0
ERA001 1 1 0 0 0
SIM102 1 1 0 0 0
S607 1 1 0 0 0
TRY300 1 1 0 0 0
PTH101 1 1 0 0 0
PTH123 1 1 0 0 0
ARG004 1 1 0 0 0
UP038 1 1 0 0 0
DTZ007 1 1 0 0 0
N999 1 1 0 0 0
S606 1 1 0 0 0
S112 1 1 0 0 0
B019 1 1 0 0 0
S406 1 1 0 0 0
S110 1 1 0 0 0
S701 1 1 0 0 0
PLR5501 1 1 0 0 0
RET504 1 1 0 0 0
N813 1 1 0 0 0
N803 1 1 0 0 0
PERF402 1 1 0 0 0
PLC0414 1 1 0 0 0
SIM103 1 1 0 0 0
SIM117 1 1 0 0 0
PYI024 1 1 0 0 0
E701 1 1 0 0 0
PLE0307 1 1 0 0 0
PT014 1 0 1 0 0
C416 1 0 1 0 0
RUF029 1 0 1 0 0

@kaddkaka
Copy link

kaddkaka commented Nov 20, 2024

does all rules already have names today?

@kaddkaka
Copy link

  • RUF102 (noqa-by-code) warns if code is used.
  • RUF103 (noqa-by-name) warns if name is used.

Is there any request to have RUF103? Is it valuable to have?

@sbrugman
Copy link
Contributor

For review I'd suggest to split this PR into one for extending noqa comments to support human-friendly rule names and one that introduces the new noqa ruff rules.

@MichaReiser
Copy link
Member

Thanks, @Humandoodlebug, for working on this. I'll close this PR because of inactivity, and I also think it requires some design work: For example, do we want to extend noqa comments, or should we introduce a ruff-specific ignore comment (e.g., ruff: ignore[code]) that allows human-readable names? I also don't think that we should allow mixing both codes and rule names.
Thanks again for exploring adding human readable names. This PR is a great inspiration.

@MichaelOultram
Copy link

I was working to rebase in the background for @Humandoodlebug, but work/life commitments kept getting in the way.

As we mentioned in the first post, we are perfectly happy for the ruff project to take a different approach here. We at Pexip developed this patch for our specific internal needs, and decided to share based on the interest in #1773.

I'm glad the PR has given at least some inspiration, and closing seems like the right choice given the design work required.

We at Pexip will continue to use the patch internally for now. When this feature is reworked and added to ruff properly, we will migrate over and ditch our patch.

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

Successfully merging this pull request may close these issues.

6 participants