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

Remove newline after code block open #3035

Merged
merged 27 commits into from Jun 11, 2022

Conversation

saroad2
Copy link
Contributor

@saroad2 saroad2 commented Apr 25, 2022

Description

Fixes #902

Newlines are great. They help us organize our code and leave room for the coders and reviewers to breath.
However, too many newlines can make simple code gigantic and massive. This is unnecessary in a lot of cases.

I want to argue that having a newline right after def is redundant and Black should remove it.
EDIT: We decided to remove trailing newlines for all code blocks, including def

Now, the following code:

def f():

    print("No newline above me!")

    print("There is a newline above me, and that's OK!")

will be changed by Black to:

def f():
    print("No newline above me!")

    print("There is a newline above me, and that's OK!")

Checklist - did you ...

  • Add a CHANGELOG entry if necessary?
  • Add / update tests if necessary?
  • Add new / update outdated documentation?

@saroad2 saroad2 changed the title Remove newline after def Remove newline after def statement Apr 25, 2022
@github-actions
Copy link

github-actions bot commented Apr 25, 2022

diff-shades results comparing this PR (ee5888b) to main (1e55718). The full diff is available in the logs under the "Generate HTML diff report" step.

╭────────────────────────── Summary ──────────────────────────╮
│ 17 projects & 1229 files changed / 3686 changes [+22/-3664] │
│                                                             │
│ ... out of 2 226 740 lines, 10 603 files & 23 projects      │
╰─────────────────────────────────────────────────────────────╯

Differences found.

What is this? | Workflow run | diff-shades documentation

CHANGES.md Outdated Show resolved Hide resolved
@saroad2
Copy link
Contributor Author

saroad2 commented Apr 25, 2022

hey @JelleZijlstra, Thanks for the review!

I made the new feature into a preview feature.
Let me know if any other changes are necessary or if you think another there are other test cases that I didn't add.

@jpy-git
Copy link
Contributor

jpy-git commented Apr 27, 2022

I'd actually already fixed this but was waiting on #2996 to get reviewed first

Based on this comment my approach was to just add an opens_block property to Line

@property
def opens_block(self) -> bool:
    """Does this line open a new level of indentation."""
    try:
        last_leaf = self.leaves[-1]
    except IndexError:
        return False

    return last_leaf.type == token.COLON

Then the logic for all blocks (including def) is:

if (
    Preview.remove_def_trailing_newline in current_line.mode
    self.previous_line.opens_block
):
    return 0, 0

@saroad2
Copy link
Contributor Author

saroad2 commented Apr 27, 2022

Hey @jpy-git , I'm sorry for the work duplication. I didn't know you are working on it.

For now, I implemented the logic only for def statements because I wanted to see how people react to that change. Removing all trailing newlines for all code blocks seems to me like a little bit too big of a change and I thought maybe some users might have a good idea why it is not good to remove trailing newlines.

Let's wait for the maintainers to react to this PR and see which approach they prefer. If they choose to remove all trailing newlines from all code blocks, I'll change the implementation to what you suggested :)

Thanks!

@felix-hilden
Copy link
Collaborator

felix-hilden commented May 7, 2022

As discussed on the issue, I'd be open to removing all newlines at the beginning of blocks. I don't see a situation where it would be better to leave them in. Others?

@saroad2
Copy link
Contributor Author

saroad2 commented May 8, 2022

Thank for the feedback @felix-hilden .

Let's see what other reviewers say and if all agree, I will make the change for all blocks using @jpy-git suggestion.

@saroad2
Copy link
Contributor Author

saroad2 commented May 18, 2022

Hey everyone, checking in on this PR.

What did we decide? make the change for all code blocks, or keeping it just for def statements for now?

@cooperlees , @ichard26 , I would love to hear your opinion on this.

@JelleZijlstra
Copy link
Collaborator

JelleZijlstra commented May 18, 2022

I'm fine with always removing them.

@ichard26
Copy link
Collaborator

ichard26 commented May 18, 2022

What did we decide? make the change for all code blocks, or keeping it just for def statements for now?

I'm in favour of making the change for all syntactic blocks. Just make sure that code style documentation covers this change. (if changes are needed, please edit the future style documentation)

@saroad2
Copy link
Contributor Author

saroad2 commented May 18, 2022

It seems like there is a consensus on removing new lines after all code blocks opening, so I'll go ahead and update the PR 🎉 .

One small request, if I might: I would love if you could merge #3062 first so I won't have to add the new test cases manually to PREVIEW_CASES. This is not a must, but it would be nice 😄

@saroad2 saroad2 changed the title Remove newline after def statement Remove newline after code block open May 18, 2022
@saroad2
Copy link
Contributor Author

saroad2 commented May 18, 2022

Well, THAT was fast!

Thanks @JelleZijlstra !

@felix-hilden
Copy link
Collaborator

felix-hilden commented May 19, 2022

Yup, a glob or something will do the trick so that we can have arbitrary nesting! But would you be ok with doing the tests our current way for this PR, and then we can discuss this in another one, as you said? We could then convert more tests and also have time for other maintainers to weigh in.

@saroad2
Copy link
Contributor Author

saroad2 commented May 19, 2022

@felix-hilden , as much as it bugs me to do so, I'll do it.

@saroad2
Copy link
Contributor Author

saroad2 commented May 19, 2022

OK @felix-hilden , I merged the test cases and trimmed down the description of the new feature in the Future Style document.

The only thing left is for other maintainers to review this PR and decide what to do about class blocks.

Copy link
Collaborator

@felix-hilden felix-hilden left a comment

Provided that we agree to remove newlines in class bodies, I think this would be good to go!

src/black/mode.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@JelleZijlstra JelleZijlstra left a comment

Looks good, just some small feedback.

Would also be good to have tests for match/case.

docs/the_black_code_style/future_style.md Outdated Show resolved Hide resolved
@saroad2
Copy link
Contributor Author

saroad2 commented May 26, 2022

Hey @JelleZijlstra , thanks for the review!

Fixed everything you've mentioned.

@saroad2
Copy link
Contributor Author

saroad2 commented Jun 1, 2022

Hey @ichard26 ,
I believe this PR is waiting for your review. If there's something missing from this PR, please let me know.

I would love to see it merged!

Copy link
Collaborator

@ichard26 ichard26 left a comment

I still haven't looked into the test infrastructure improvements, but everything else looks great! I'm so sorry I took so long to review this, thank you so much for waiting!

@@ -148,6 +148,7 @@ Multiple contributions by:
- [Rishikesh Jha](mailto:rishijha424@gmail.com)
- [Rupert Bedford](mailto:rupert@rupertb.com)
- Russell Davis
- [Sagi Shadur](mailto:saroad2@gmail.com)
Copy link
Collaborator

@ichard26 ichard26 Jun 11, 2022

Choose a reason for hiding this comment

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

This AUTHORS list is so out of date, gosh. Good for you adding yourself!

@@ -1455,7 +1455,6 @@ def test_newline_comment_interaction(self) -> None:
black.assert_stable(source, output, mode=DEFAULT_MODE)

def test_bpo_2142_workaround(self) -> None:

Copy link
Collaborator

@ichard26 ichard26 Jun 11, 2022

Choose a reason for hiding this comment

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

Haha, I like it!

@@ -49,3 +49,28 @@ plain strings. User-made splits are respected when they do not exceed the line l
limit. Line continuation backslashes are converted into parenthesized strings.
Unnecessary parentheses are stripped. The stability and status of this feature is
tracked in [this issue](https://github.com/psf/black/issues/2188).

### Removing trailing newlines after code block open
Copy link
Collaborator

@ichard26 ichard26 Jun 11, 2022

Choose a reason for hiding this comment

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

Oh lovely, I've been worried lately our style documentation is out of date with the current state of affairs, but this quells my worries with this PR ^^

@felix-hilden
Copy link
Collaborator

felix-hilden commented Jun 11, 2022

With three approvals, I think it's high time we get this in! Thanks for the contribution, responsiveness and patience 😃 Also, I don't remember if this has been linked to you since this isn't your first PR, but Rich has been gathering feedback from contributors in #2238 We'd appreciate any feedback there!

@felix-hilden felix-hilden merged commit 4bb7bf2 into psf:main Jun 11, 2022
40 checks passed
@saroad2 saroad2 deleted the remove_newline_after_def branch Jun 11, 2022
felix-hilden added a commit to felix-hilden/black that referenced this pull request Jun 24, 2022
ichard26 added a commit that referenced this pull request Jun 27, 2022
Covers GH-2926, GH-2990, GH-2991, and GH-3035.

Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
Co-authored-by: Richard Si <63936253+ichard26@users.noreply.github.com>
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.

5 participants