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

[pycodestyle] Do not ignore lines before the first logical line in blank lines rules. #10382

Merged

Conversation

hoel-bagard
Copy link
Contributor

@hoel-bagard hoel-bagard commented Mar 13, 2024

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 to E302.

For example, in the snippet below a rule violation should be detected on the second comment and on the import.

# first comment




# second comment




import foo

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.

Copy link

github-actions bot commented Mar 13, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

Formatter (stable)

✅ ecosystem check detected no format changes.

Formatter (preview)

✅ ecosystem check detected no format changes.

Ignoring all lines until the first logical line does not match the
behavior from pycodestyle.

Fixes astral-sh#10374
@hoel-bagard hoel-bagard force-pushed the hoel/blank-lines-handle-first-line branch from feb5e45 to 78ca1d2 Compare March 13, 2024 13:38
@hoel-bagard hoel-bagard marked this pull request as ready for review March 13, 2024 13:52
Copy link
Member

@dhruvmanila dhruvmanila left a 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.

@dhruvmanila dhruvmanila added the bug Something isn't working label Mar 13, 2024
@dhruvmanila
Copy link
Member

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 main and passes on this branch :)

@hoel-bagard
Copy link
Contributor Author

@dhruvmanila I've added a few fixtures as you requested. Since E302 and E303 are the only rules that should be able to trigger on the second line, I only added snapshots for those rules.

Should I also add snapshots for the other rules to check that no false positive is introduced in the future ?

Copy link
Member

@dhruvmanila dhruvmanila left a 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

@dhruvmanila dhruvmanila merged commit e944c16 into astral-sh:main Mar 14, 2024
17 checks passed
@hoel-bagard hoel-bagard deleted the hoel/blank-lines-handle-first-line branch March 14, 2024 12:26
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.

E303: missing detection
2 participants