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
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 3 additions & 0 deletions CHANGES.md
Expand Up @@ -7,6 +7,9 @@
- `Black` now respects `--skip-string-normalization` when normalizing multiline
docstring quotes (#1637)

- `Black` no longer removes all empty lines between non-function code and decorators
when formatting typing stubs. Now `Black` enforces a single empty line. (#1646)

- fixed a crash when PWD=/ on POSIX (#1631)

### 20.8b1
Expand Down
5 changes: 5 additions & 0 deletions docs/change_log.md
Expand Up @@ -9,6 +9,11 @@
- `Black` now respects `--skip-string-normalization` when normalizing multiline
docstring quotes (#1637)

- `Black` no longer removes all empty lines between non-function code and decorators
when formatting typing stubs. Now `Black` enforces a single empty line. (#1646)

- fixed a crash when PWD=/ on POSIX (#1631)

### 20.8b1

#### _Packaging_
Expand Down
11 changes: 9 additions & 2 deletions src/black/__init__.py
Expand Up @@ -1824,6 +1824,10 @@ def _maybe_empty_lines_for_class_or_def(
return 0, 0

if self.previous_line.is_decorator:
if self.is_pyi and current_line.is_stub_class:
# Insert an empty line after a decorated stub class
return 0, 1

return 0, 0

if self.previous_line.depth < current_line.depth and (
Expand All @@ -1847,8 +1851,11 @@ def _maybe_empty_lines_for_class_or_def(
newlines = 0
else:
newlines = 1
elif current_line.is_def and not self.previous_line.is_def:
# Blank line between a block of functions and a block of non-functions
elif (
current_line.is_def or current_line.is_decorator
) and not self.previous_line.is_def:
Comment on lines +1854 to +1856
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.

# Blank line between a block of functions (maybe with preceding
# decorators) and a block of non-functions
newlines = 1
else:
newlines = 0
Expand Down
67 changes: 63 additions & 4 deletions tests/data/force_pyi.py
@@ -1,6 +1,65 @@
def f(): ...
from typing import Union

@bird
def zoo(): ...

class A: ...
@bar
class B:
def BMethod(self) -> None: ...
@overload
def BMethod(self, arg : List[str]) -> None: ...

class C: ...
@hmm
class D: ...
class E: ...

@baz
def foo() -> None:
...

class F (A , C): ...
def spam() -> None: ...

@overload
def spam(arg: str) -> str: ...

var : int = 1

def eggs() -> Union[str, int]: ...

def g(): ...
# output
def f(): ...
def g(): ...

from typing import Union

@bird
def zoo(): ...

class A: ...

@bar
class B:
def BMethod(self) -> None: ...
@overload
def BMethod(self, arg: List[str]) -> None: ...

class C: ...

@hmm
class D: ...

class E: ...

@baz
def foo() -> None: ...

class F(A, C): ...

def spam() -> None: ...
@overload
def spam(arg: str) -> str: ...

var: int = 1

def eggs() -> Union[str, int]: ...
7 changes: 4 additions & 3 deletions tests/test_black.py
Expand Up @@ -1527,7 +1527,6 @@ def test_tricky_unicode_symbols(self) -> None:
black.assert_stable(source, actual, DEFAULT_MODE)

def test_single_file_force_pyi(self) -> None:
reg_mode = DEFAULT_MODE
pyi_mode = replace(DEFAULT_MODE, is_pyi=True)
contents, expected = read_data("force_pyi")
with cache_dir() as workspace:
Expand All @@ -1540,9 +1539,11 @@ def test_single_file_force_pyi(self) -> None:
# verify cache with --pyi is separate
pyi_cache = black.read_cache(pyi_mode)
self.assertIn(path, pyi_cache)
normal_cache = black.read_cache(reg_mode)
normal_cache = black.read_cache(DEFAULT_MODE)
self.assertNotIn(path, normal_cache)
self.assertEqual(actual, expected)
self.assertFormatEqual(expected, actual)
black.assert_equivalent(contents, actual)
black.assert_stable(contents, actual, pyi_mode)

@event_loop()
def test_multi_file_force_pyi(self) -> None:
Expand Down