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

E704 and concise formatting for dummy implementations #3887

Closed
hauntsaninja opened this issue Sep 13, 2023 · 5 comments
Closed

E704 and concise formatting for dummy implementations #3887

hauntsaninja opened this issue Sep 13, 2023 · 5 comments
Labels
C: preview style Issues with the preview and unstable style. Add the name of the responsible feature in the title. F: empty lines Wasting vertical space efficiently. T: bug Something isn't working

Comments

@hauntsaninja
Copy link
Collaborator

This preview style change #3796 violates E704 as implemented by flake8 / pycodestyle.

Note that the change itself is PEP 8 compliant. E704 is disabled by default in pycodestyle for this reason. ruff intentionally does not implement E704, instead explicitly deferring to Black on this question.

At a minimum we should update https://black.readthedocs.io/en/stable/the_black_code_style/current_style.html#flake8 , but figured I'd open an issue to discuss.

@hauntsaninja hauntsaninja added the T: bug Something isn't working label Sep 13, 2023
@cooperlees
Copy link
Collaborator

I'm not a fan of this change, but can live with it. I do agree with the removing of the new lines after a class definition here, but I don't see the benefit with overloads stated in the issue within these Abstract Base classes etc.

This has made some projects I maintain require a change to line number set in tests for flake8-bugbear unittests and have set the formatting off for a chunk of bandersnatch code as I don't find it easier to read. Saving vertical space has never been a goal of black.

I tend to agree with flake8's E704 here. I like not to put multiple statements on a line. But if we stick with this we will be forcing people to have to add exempting E704 in their flake8 configs on projects. This will cause CI to miss other multiple statements per line on black formatted projects if this behavior is maintained.

@cooperlees cooperlees added F: empty lines Wasting vertical space efficiently. C: preview style Issues with the preview and unstable style. Add the name of the responsible feature in the title. labels Sep 13, 2023
@hauntsaninja
Copy link
Collaborator Author

I'm a little confused by the last paragraph. E704 is something that is typically completely obviated by an autoformatter (which is e.g. why ruff doesn't implement the check). So the only "other multiple statements per line" I could see CI missing are due to disabling autoformatting (or due to bugs in Black). If someone wants multiple statements on a line between # fmt: off and # fmt: on, but still wants flake8 to warn them about that, that feels a little adverse.

I don't expect formatting changes to be uncontroversial. But I was hopeful, since in addition to my personal preference, #1797 is fairly popular, PEP 8 itself uses this kind of formatting in a couple places, and pycodestyle also disables E704 by default.

I'll update the docs, so that main is at least self-consistent. Hopefully --preview users give us some more feedback and we get a better sense of whether this change should be modified or rolled back.

hauntsaninja added a commit to hauntsaninja/black that referenced this issue Sep 13, 2023
@JelleZijlstra
Copy link
Collaborator

For what it's worth, I would find the linked bandersnatch code more readable with Black's new formatting. Fewer lines make it easier to see the whole protocol at a glance. For the same reason, we've always formatted stubs in typeshed this way.

On flake8-bugbear the change is annoying because so many line numbers have to change, but that's a risk of that project's test setup.

hauntsaninja added a commit that referenced this issue Sep 13, 2023
@cooperlees
Copy link
Collaborator

Rightio - I'll conform.

@JelleZijlstra
Copy link
Collaborator

We decided to keep the change. There is an open issue about updating docs related to E704.

JonathanWillitts added a commit to clinicedc/edc-action-item that referenced this issue Feb 2, 2024
Black changes witnessed (mainly) include:
- addition of blank line between module docstrings and imports
- removal of blank lines between class declarations and docstrings
- prefer splitting assignment statements on right-hand side
- wrap conditional expressions that span multiple lines in parens
- put `...` in stubs on same line
- add E701 and E704 to flake8 ignore list, see:
    - https://github.com/psf/black/blob/main/docs/guides/using_black_with_other_tools.md#e701--e704
    - psf/black#3887 (E704)
    - psf/black#4173 (E701)
JonathanWillitts added a commit to clinicedc/edc-visit-schedule that referenced this issue Feb 2, 2024
Black changes witnessed (mainly) include:
- addition of blank line between module docstrings and imports
- removal of blank lines between class declarations and docstrings
- prefer splitting assignment statements on right-hand side
- wrap conditional expressions that span multiple lines in parens
- put `...` in stubs on same line
- add E701 and E704 to flake8 ignore list, see:
    - https://github.com/psf/black/blob/main/docs/guides/using_black_with_other_tools.md#e701--e704
    - psf/black#3887 (E704)
    - psf/black#4173 (E701)
JonathanWillitts added a commit to clinicedc/edc-visit-tracking that referenced this issue Feb 2, 2024
Black changes witnessed (mainly) include:
- addition of blank line between module docstrings and imports
- removal of blank lines between class declarations and docstrings
- prefer splitting assignment statements on right-hand side
- wrap conditional expressions that span multiple lines in parens
- put `...` in stubs on same line
- add E701 and E704 to flake8 ignore list, see:
    - https://github.com/psf/black/blob/main/docs/guides/using_black_with_other_tools.md#e701--e704
    - psf/black#3887 (E704)
    - psf/black#4173 (E701)
JonathanWillitts added a commit to clinicedc/edc-metadata that referenced this issue Feb 2, 2024
Black changes witnessed (mainly) include:
- addition of blank line between module docstrings and imports
- removal of blank lines between class declarations and docstrings
- prefer splitting assignment statements on right-hand side
- wrap conditional expressions that span multiple lines in parens
- put `...` in stubs on same line
- add E701 and E704 to flake8 ignore list, see:
    - https://github.com/psf/black/blob/main/docs/guides/using_black_with_other_tools.md#e701--e704
    - psf/black#3887 (E704)
    - psf/black#4173 (E701)
JonathanWillitts added a commit to clinicedc/edc-model that referenced this issue Feb 2, 2024
Black changes witnessed (mainly) include:
- addition of blank line between module docstrings and imports
- removal of blank lines between class declarations and docstrings
- prefer splitting assignment statements on right-hand side
- wrap conditional expressions that span multiple lines in parens
- put `...` in stubs on same line
- add E701 and E704 to flake8 ignore list, see:
    - https://github.com/psf/black/blob/main/docs/guides/using_black_with_other_tools.md#e701--e704
    - psf/black#3887 (E704)
    - psf/black#4173 (E701)
agateau-gg added a commit to GitGuardian/ggshield that referenced this issue Apr 3, 2024
This required disabling E704 in flake8. That seems to be expected:
psf/black#3887
agateau-gg added a commit to GitGuardian/ggshield that referenced this issue Apr 3, 2024
This required disabling E704 in flake8. That seems to be expected:
psf/black#3887
skim0119 added a commit to skim0119/PyElastica that referenced this issue Apr 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C: preview style Issues with the preview and unstable style. Add the name of the responsible feature in the title. F: empty lines Wasting vertical space efficiently. T: bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants