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

Fix E30X panics on blank lines with trailing white spaces #9907

Merged
merged 2 commits into from Feb 9, 2024

Conversation

hoel-bagard
Copy link
Contributor

@hoel-bagard hoel-bagard commented Feb 9, 2024

Summary

closes #9906
The #9906 issue is triggered by blank lines with trailing white spaces. The trailing spaces cause the assert_eq! that was added in this discussion to fail.

This PR removes the assert_eq!.

Test Plan

I tested the change on the python files provided in #9906 (on top of cargo test).
I also added a few fixtures with blank lines with trailing white spaces.

Issue example/explaination

The issue is triggered by blank lines with trailing white spaces.
For example, by creating a blank file with echo "\n \n" >> temp.py, we get the snippet given in the issue:


     

Running ruff on this, we get the following tokens:

(NonLogicalNewline, 0..1)
(NonLogicalNewline, 6..7)
(NonLogicalNewline, 7..8)

We can see that the end of the first token range's end and the second token range's start do not match because of the 5 spaces.

Copy link

github-actions bot commented Feb 9, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

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

aws/aws-sam-cli (+33 -0 violations, +0 -0 fixes)

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

+ tests/integration/testdata/start_api/lambda_authorizers/app.py:122:17: E203 [*] Whitespace before ':'
+ tests/integration/testdata/start_api/lambda_authorizers/app.py:123:15: E203 [*] Whitespace before ':'
+ tests/integration/testdata/start_api/lambda_authorizers/app.py:131:8: E221 [*] Multiple spaces before operator
+ tests/integration/testdata/start_api/lambda_authorizers/app.py:132:9: E221 [*] Multiple spaces before operator
+ tests/integration/testdata/start_api/lambda_authorizers/app.py:133:8: E221 [*] Multiple spaces before operator
+ tests/integration/testdata/start_api/lambda_authorizers/app.py:134:10: E221 [*] Multiple spaces before operator
+ tests/integration/testdata/start_api/lambda_authorizers/app.py:135:9: E221 [*] Multiple spaces before operator
+ tests/integration/testdata/start_api/lambda_authorizers/app.py:136:11: E221 [*] Multiple spaces before operator
+ tests/integration/testdata/start_api/lambda_authorizers/app.py:138:8: E221 [*] Multiple spaces before operator
+ tests/integration/testdata/start_api/lambda_authorizers/app.py:140:1: E302 [*] Expected 2 blank lines, found 1
+ tests/integration/testdata/start_api/lambda_authorizers/app.py:14:1: I001 [*] Import block is un-sorted or un-formatted
+ tests/integration/testdata/start_api/lambda_authorizers/app.py:159:5: E303 [*] Too many blank lines (2)
+ tests/integration/testdata/start_api/lambda_authorizers/app.py:17:1: E302 [*] Expected 2 blank lines, found 1
+ tests/integration/testdata/start_api/lambda_authorizers/app.py:203:30: E203 [*] Whitespace before ':'
+ tests/integration/testdata/start_api/lambda_authorizers/app.py:204:29: E203 [*] Whitespace before ':'
+ tests/integration/testdata/start_api/lambda_authorizers/app.py:208:30: E203 [*] Whitespace before ':'
+ tests/integration/testdata/start_api/lambda_authorizers/app.py:209:29: E203 [*] Whitespace before ':'
+ tests/integration/testdata/start_api/lambda_authorizers/app.py:212:9: PLR6301 Method `_getEmptyStatement` could be a function, class method, or static method
+ tests/integration/testdata/start_api/lambda_authorizers/app.py:284:26: E203 [*] Whitespace before ':'
+ tests/integration/testdata/start_api/lambda_authorizers/app.py:285:29: E203 [*] Whitespace before ':'
+ tests/integration/testdata/start_api/lambda_authorizers/app.py:286:26: E203 [*] Whitespace before ':'
... 8 additional changes omitted for rule E203
+ tests/integration/testdata/start_api/lambda_authorizers/app.py:34:1: E302 [*] Expected 2 blank lines, found 1
+ tests/integration/testdata/start_api/lambda_authorizers/app.py:44:1: E302 [*] Expected 2 blank lines, found 1
+ tests/integration/testdata/start_api/lambda_authorizers/app.py:47:1: E302 [*] Expected 2 blank lines, found 1
+ tests/integration/testdata/start_api/lambda_authorizers/app.py:59:1: E302 [*] Expected 2 blank lines, found 1
+ tests/integration/testdata/start_api/lambda_authorizers/app.py:68:1: E302 [*] Expected 2 blank lines, found 1

Changes by rule (6 rules affected)

code total + violation - violation + fix - fix
E203 16 16 0 0 0
E221 7 7 0 0 0
E302 7 7 0 0 0
I001 1 1 0 0 0
E303 1 1 0 0 0
PLR6301 1 1 0 0 0

@hoel-bagard hoel-bagard marked this pull request as ready for review February 9, 2024 09:46
@MichaReiser
Copy link
Member

Thanks for looking into the panic. I'm reluctant to just remove the assertion without better understanding the reason why it is incorrect (I added it to guarantee it's only called on two directly following lines). Would you mind adding an example explaining why the two ranges don't end and start at the same position to the PR summary?

@MichaReiser MichaReiser added the bug Something isn't working label Feb 9, 2024
@hoel-bagard
Copy link
Contributor Author

@MichaReiser This happens because of the whitespaces, I added an explanation to the PR summary.

@MichaReiser MichaReiser merged commit 12a91f4 into astral-sh:main Feb 9, 2024
17 checks passed
@MichaReiser
Copy link
Member

Thanks for the explanation. That makes sense

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rules E30X panics with almost empty files
2 participants