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

Revert "[pylint] Implement import-private-name (C2701)" #9547

Merged
merged 1 commit into from
Jan 16, 2024

Conversation

MichaReiser
Copy link
Member

Reverts #5920 because It's panicking in the pydantic benchmark run (see astral-sh/ruff/actions/runs/7537305800/job/20516059364?pr=5920).

ruff on  lexer-explicit-id-false-brnaches [$] is 📦 v0.1.13 via 🐍 v3.11.6 via 🦀 v1.75.0 
❯ cargo bench -p ruff_benchmark --bench linter --  "all-with-preview-rules[pydantic/types.py]"
    Finished bench [optimized] target(s) in 0.08s
     Running benches/linter.rs (target/release/deps/linter-3478ab4e9ed35ca9)
linter/all-with-preview-rules/numpy/globals.py
                        time:   [165.16 µs 165.62 µs 166.37 µs]
                        thrpt:  [17.736 MiB/s 17.816 MiB/s 17.866 MiB/s]
Found 6 outliers among 100 measurements (6.00%)
  1 (1.00%) high mild
  5 (5.00%) high severe
linter/all-with-preview-rules/unicode/pypinyin.py
                        time:   [657.40 µs 657.83 µs 658.47 µs]
                        thrpt:  [6.3813 MiB/s 6.3875 MiB/s 6.3917 MiB/s]
Found 11 outliers among 100 measurements (11.00%)
  6 (6.00%) high mild
  5 (5.00%) high severe
linter/all-with-preview-rules/pydantic/types.py
                        time:   [2.5630 ms 2.5687 ms 2.5749 ms]
                        thrpt:  [9.9046 MiB/s 9.9285 MiB/s 9.9506 MiB/s]
Found 20 outliers among 100 measurements (20.00%)
  18 (18.00%) high mild
  2 (2.00%) high severe
Benchmarking linter/all-with-preview-rules/numpy/ctypeslib.py: Warming up for 3.0000 sthread 'main' panicked at crates/ruff_linter/src/rules/pylint/rules/import_private_name.rs:137:41:
range end index 2 out of range for slice of length 1
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

error: bench failed, to rerun pass `-p ruff_benchmark --bench linter`

@tjkuson would you mind having a look? I think it's due to the slicing into module_name

@MichaReiser MichaReiser added the rule Implementing or modifying a lint rule label Jan 16, 2024
@MichaReiser MichaReiser enabled auto-merge (squash) January 16, 2024 08:28
@MichaReiser MichaReiser merged commit f9191b0 into main Jan 16, 2024
16 checks passed
@MichaReiser MichaReiser deleted the revert-5920-import-private-name branch January 16, 2024 08:33
Copy link

codspeed-hq bot commented Jan 16, 2024

CodSpeed Performance Report

Merging #9547 will improve performances by 4.66%

⚠️ No base runs were found

Falling back to comparing revert-5920-import-private-name (b045ee1) with main (7ef7e0d)

Summary

⚡ 1 improvements
✅ 29 untouched benchmarks

Benchmarks breakdown

Benchmark main revert-5920-import-private-name Change
parser[numpy/ctypeslib.py] 12.8 ms 12.2 ms +4.66%

Copy link
Contributor

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

@tjkuson
Copy link
Contributor

tjkuson commented Jan 16, 2024

@MichaReiser I can submit a patch this evening, sorry for any disruption!

@MichaReiser
Copy link
Member Author

MichaReiser commented Jan 16, 2024

@MichaReiser I can submit a patch this evening, sorry for any disruption!

No worries at all and thanks for taking a look. This is also not urgent (that's why I reverted, there's no need to rush it)

charliermarsh pushed a commit that referenced this pull request Jan 16, 2024
## Summary

#5920 with a fix for the erroneous slice in `module_name`. Fixes #9547.

## Test Plan

Added `import bbb.ccc._ddd as eee` to the test fixture to ensure it no
longer panics.

`cargo test`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rule Implementing or modifying a lint rule
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants