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 E265 and E266 confusion and overlap #650

Merged
merged 7 commits into from Sep 23, 2022

Conversation

PeterJCLaw
Copy link
Contributor

@PeterJCLaw PeterJCLaw commented Sep 16, 2022

This rewrites the E265 and E266 fixers to be more like the majority of the other fixers that autopep8 has (namely that they're now members of the FixPEP8 class). In doing so it separates how they work, making it possible for users to have neither, either or both fixers enabled.

Reviewing by commit may be useful to understand the changes.

Fixes #649.

The codecov miss here is within apply_global_fixes which is doing less now that it really only exists for W602.

E265 is "block comment should start with '# '"
E266 is "too many leading '#' for block comment"
It still ends up actually fixing E265 as well (it always did both)
however that one is going to be easier to reproduce as a standard
fixer.
The E266 fixer will soon only fix E266, so E265 will need its own handling.
This detatches this from the implementation function, clearing the
way for changing the implementation.
@PeterJCLaw PeterJCLaw force-pushed the fix-e265-e266-confusion branch 2 times, most recently from adf1353 to 0a2c8e3 Compare September 16, 2022 19:21
This still ends up fixing cases of E265 which overlap with E266
but doesn't go out of its way to do so if E265 is disabled. This
ought to be fine since pycodestyle doesn't seem to ever complain
about E266 on any lines affected by E265.
Previously it would end up changing E266 as well, which the user
may not want it to do (for example if they've disabled E266).

This also adds a battery of tests to excercise the differences
between these errors.
This appears to be expected for compatibility, though is a somewhat
unexpected behaviour given that these comments are almost certainly
not actually 'shebang' comments.
@codecov-commenter
Copy link

Codecov Report

Base: 97.95% // Head: 97.91% // Decreases project coverage by -0.04% ⚠️

Coverage data is based on head (f4a40b1) compared to base (4884136).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #650      +/-   ##
==========================================
- Coverage   97.95%   97.91%   -0.05%     
==========================================
  Files           1        1              
  Lines        2398     2399       +1     
==========================================
  Hits         2349     2349              
- Misses         49       50       +1     
Impacted Files Coverage Δ
autopep8.py 97.91% <100.00%> (-0.05%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@PeterJCLaw PeterJCLaw marked this pull request as ready for review September 16, 2022 19:52
@hhatto hhatto self-requested a review September 17, 2022 01:23
Copy link
Owner

@hhatto hhatto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Thanks for your contribution 👍

@hhatto hhatto merged commit 02bcfbf into hhatto:main Sep 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

E266 cannot be ignored on its own
3 participants