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 empty line handling when formatting typing stubs #1646

Merged
merged 2 commits into from Sep 10, 2020

Conversation

ichard26
Copy link
Collaborator

Fixes #1490.

Black used to erroneously remove all empty lines between functions
preceded by decorators and non-function code in pyi mode. Now a single
empty line is enforced.

Comment on lines +1850 to +1856
elif (
current_line.is_def or current_line.is_decorator
) and not self.previous_line.is_def:
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Right now on master Black enforces this formatting:

class A: ...

@bar
class B: ...
    def BMethod(self) -> Any: ...

class C: ...

While the empty line between class B and C is right, I am not sure if the empty line between class A and class B is right since class A has an empty body. That empty line happens since Black can't tell the difference between the previous class not having an empty body or the current class being preceded by a decorator. Currently this commit keeps this behaviour since as far as I know classes don't have a reason to be decorated in stubs, but no idea whether that's right or not...

/cc @ambv @JelleZijlstra (I barely have any experience with typing stubs so your knowledge would be appreciated)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would put a blank line there. Nontrivial classes (with a body that's not ...) should have an empty line on either side.

Copy link
Collaborator

Choose a reason for hiding this comment

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

And there are some use cases for decorating classes in stubs, like @runtime_checkable for protocols.

Copy link
Collaborator Author

@ichard26 ichard26 Aug 28, 2020

Choose a reason for hiding this comment

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

Sorry, I meant for the case of:

class A: ...

@bar
class B: ...
class C: ...

The empty line between class A and B doesn't seem right as A and B have empty bodies. Master and this commit enforces this formatting, but I feel like it's wrong since if that decorator wasn't there, that empty line would disappear.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh I see, I could go either way on that. I feel like if we do add blank lines, it should have blank lines both above and below it.

@ichard26 ichard26 marked this pull request as draft August 28, 2020 04:15
@ichard26 ichard26 marked this pull request as ready for review August 29, 2020 02:08
Black used to erroneously remove all empty lines between non-function
code and decorators when formatting typing stubs. Now a single empty
line is enforced.

I chose for putting empty lines around decorated classes that have empty
bodies since removing empty lines around such classes would cause a
formatting issue that seems to be impossible to fix.

For example:

```
class A: ...
@some_decorator
class B: ...
class C: ...
class D: ...

@some_other_decorator
def foo(): -> None: ...
```

It is easy to enforce no empty lines between class A, B, and C.
Just return 0, 0 for a line that is a decorator and precedes an stub
class. Fortunately before this commit, empty lines after that class
would be removed already.

Now let's look at the empty line between class D and function foo. In
this case, there should be an empty line there since it's class code next
to function code. The problem is that when deciding to add X empty lines
before a decorator, you can't tell whether it's before a class or a
function. If the decorator is before a function, then an empty line
is needed, while no empty lines are needed when the decorator is
before a class.

So even though I personally prefer no empty lines around decorated
classes, I had to go the other way surrounding decorated classes with
empty lines.
@JelleZijlstra
Copy link
Collaborator

I fixed the merge conflict, now there are some primer failures which look unrelated to this change; they're in flake8-bugbear in non-stub code.

@hugovk
Copy link
Contributor

hugovk commented Sep 10, 2020

Flake8-bugbear is now Black-formatted (PyCQA/flake8-bugbear#136 (some familiar names there!)), restarting the Primer run should hopefully be green now.

@JelleZijlstra
Copy link
Collaborator

Now it's failing on virtualenv, likely because pypa/virtualenv@13c0498 reformatted it.

@hugovk
Copy link
Contributor

hugovk commented Sep 10, 2020

PR #1695 flips the Primer switch for virtualenv.

@JelleZijlstra JelleZijlstra reopened this Sep 10, 2020
@JelleZijlstra JelleZijlstra merged commit c0a8e42 into psf:master Sep 10, 2020
@ichard26 ichard26 deleted the fix_spacing_in_pyi_mode branch November 1, 2020 17:17
noxan pushed a commit to noxan/black that referenced this pull request Jun 6, 2021
Black used to erroneously remove all empty lines between non-function
code and decorators when formatting typing stubs. Now a single empty
line is enforced.

I chose for putting empty lines around decorated classes that have empty
bodies since removing empty lines around such classes would cause a
formatting issue that seems to be impossible to fix.

For example:

```
class A: ...
@some_decorator
class B: ...
class C: ...
class D: ...

@some_other_decorator
def foo(): -> None: ...
```

It is easy to enforce no empty lines between class A, B, and C.
Just return 0, 0 for a line that is a decorator and precedes an stub
class. Fortunately before this commit, empty lines after that class
would be removed already.

Now let's look at the empty line between class D and function foo. In
this case, there should be an empty line there since it's class code next
to function code. The problem is that when deciding to add X empty lines
before a decorator, you can't tell whether it's before a class or a
function. If the decorator is before a function, then an empty line
is needed, while no empty lines are needed when the decorator is
before a class.

So even though I personally prefer no empty lines around decorated
classes, I had to go the other way surrounding decorated classes with
empty lines.

Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.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.

black removes empty line between imports and overloads in stubs
3 participants