-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
[pycodestyle
] Do not ignore lines before the first logical line in blank lines rules.
#10382
[pycodestyle
] Do not ignore lines before the first logical line in blank lines rules.
#10382
Conversation
|
Ignoring all lines until the first logical line does not match the behavior from pycodestyle. Fixes astral-sh#10374
feb5e45
to
78ca1d2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow, thanks for the quick fix. Can you add some test cases for this? Ideally, separate test cases which tests against comments, docstrings, a random statement and expression.
I agree with your test plan although I think it would be useful to avoid any potential regression in the future. It's always good to have a test case for a bug fix PR which fails on |
@dhruvmanila I've added a few fixtures as you requested. Since Should I also add snapshots for the other rules to check that no false positive is introduced in the future ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! The test cases are great
Summary
Ignoring all lines until the first logical line does not match the behavior from pycodestyle. This PR therefore removes the
if state.is_not_first_logical_line
skipping the line check before the first logical line, and applies it only toE302
.For example, in the snippet below a rule violation should be detected on the second comment and on the import.
Fixes #10374
Test Plan
It was tested on the example given on the issue, and by running cargo test. Since this is an issue about the first line(s) of a file I did not add a fixture for it.
The ruff-ecosystem check should help verify that the PR does not have unintended side-effects.