Skip to content

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

Merged
MichaReiser merged 1 commit intomainfrom
revert-5920-import-private-name
Jan 16, 2024
Merged

Revert "[pylint] Implement import-private-name (C2701)"#9547
MichaReiser merged 1 commit intomainfrom
revert-5920-import-private-name

Conversation

@MichaReiser
Copy link
Copy Markdown
Member

@MichaReiser MichaReiser commented Jan 16, 2024

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
@MichaReiser MichaReiser deleted the revert-5920-import-private-name branch January 16, 2024 08:33
@codspeed-hq
Copy link
Copy Markdown

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%

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Jan 16, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

@tjkuson
Copy link
Copy Markdown
Contributor

tjkuson commented Jan 16, 2024

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

@MichaReiser
Copy link
Copy Markdown
Member Author

MichaReiser commented Jan 16, 2024 •
edited
Loading

@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)

@tjkuson tjkuson mentioned this pull request Jan 16, 2024
Merged
charliermarsh pushed a commit that referenced this pull request Jan 16, 2024
@tjkuson
[pylint] (Re-)Implement import-private-name (C2701) (#9553)
f426c0f 
## 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

Reviewers

No reviews

Assignees

No one assigned

Labels

rule Implementing or modifying a lint rule

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@MichaReiser @tjkuson