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

Put blank lines between nested classes in typing stubs #2783

Closed
MarkKoz opened this issue Jan 19, 2022 · 6 comments · Fixed by #3564
Closed

Put blank lines between nested classes in typing stubs #2783

MarkKoz opened this issue Jan 19, 2022 · 6 comments · Fixed by #3564
Labels
F: empty lines Wasting vertical space efficiently. S: accepted The changes in this design / enhancement issue have been accepted and can be implemented T: style What do we want Blackened code to look like?

Comments

@MarkKoz
Copy link

MarkKoz commented Jan 19, 2022

Describe the style change

Nested classes in typing stubs should have blank lines between them.

Examples in the current Black style

class A:
    class B:
        attribute: int
    class C:
        attribute: int
    def method(self) -> None: ...
    def method2(self) -> None: ...

Desired style

class A:
    class B:
        attribute: int
    
    class C:
        attribute: int
    
    def method(self) -> None: ...
    def method2(self) -> None: ...

Additional context

See this for a live demo.

@MarkKoz MarkKoz added the T: style What do we want Blackened code to look like? label Jan 19, 2022
@ichard26 ichard26 added F: empty lines Wasting vertical space efficiently. S: accepted The changes in this design / enhancement issue have been accepted and can be implemented labels Jan 19, 2022
@JelleZijlstra
Copy link
Collaborator

I'm in favor of making this change, but cc fellow typeshed maintainers @srittau @Akuli @hauntsaninja.

@ichard26 ichard26 added S: needs discussion Needs further hashing out before ready for implementation (on desirability, feasibility, etc.) and removed S: accepted The changes in this design / enhancement issue have been accepted and can be implemented labels Jan 19, 2022
@hauntsaninja
Copy link
Collaborator

hauntsaninja commented Jan 19, 2022

Yeah, seems nice.

Somewhat related, the stubs style change I want from black is for top-level code under an if-statement to have the same blank lines as top-level code proper. Example here

@JelleZijlstra
Copy link
Collaborator

@hauntsaninja we made some changes around that in #2472, but agree that we should space in your example too. Feel free to open a new issue. We also have an unreleased change in #2736.

@Akuli
Copy link

Akuli commented Jan 19, 2022

I'm fine with this change.

@JelleZijlstra
Copy link
Collaborator

Everyone agrees, so @MarkKoz feel free to go ahead and implement this. Thanks!

@ichard26 ichard26 added S: accepted The changes in this design / enhancement issue have been accepted and can be implemented and removed S: needs discussion Needs further hashing out before ready for implementation (on desirability, feasibility, etc.) labels Jan 21, 2022
@jpy-git
Copy link
Contributor

jpy-git commented Mar 16, 2022

Had a quick look at this and, since there's no special consideration given to whether an indented block is within a class or an if, it seems the work in #2820 and #2796 has partially addressed this issue.
There are however there are a couple other cases that need to be accounted for in both if and class blocks: if example and class example

For stubs, it feels like making any indented block consistent with the top level achieves the goal of this issue and #2784.

Although one place I can see class and if differing is the gap between attributes and methods (example) since this may introduce a lot of changes in typeshed.

konstin added a commit to konstin/black that referenced this issue Sep 8, 2023
The idea behind this change is that we stop looking into previous body to determine if there should be a blank before a function or class definition.

Input:

```python
import sys

if sys.version_info > (3, 7):
    class Nested1:
        assignment = 1
        def function_definition(self): ...
    def f1(self) -> str: ...
    class Nested2:
        def function_definition(self): ...
        assignment = 1
    def f2(self) -> str: ...

if sys.version_info > (3, 7):
    def nested1():
        assignment = 1
        def function_definition(self): ...
    def f1(self) -> str: ...
    def nested2():
        def function_definition(self): ...
        assignment = 1
    def f2(self) -> str: ...
```

Stable style
```python
import sys

if sys.version_info > (3, 7):
    class Nested1:
        assignment = 1
        def function_definition(self): ...

    def f1(self) -> str: ...

    class Nested2:
        def function_definition(self): ...
        assignment = 1
    def f2(self) -> str: ...

if sys.version_info > (3, 7):
    def nested1():
        assignment = 1
        def function_definition(self): ...

    def f1(self) -> str: ...
    def nested2():
        def function_definition(self): ...
        assignment = 1
    def f2(self) -> str: ...
```

In the stable formatting, we have a blank line sometimes, not depending on the previous statement on the same level, but on the last (potentially nested) statement in the previous body.

psf#2783/psf#3564 fixes this for classes in preview style:

```python
import sys

if sys.version_info > (3, 7):
    class Nested1:
        assignment = 1
        def function_definition(self): ...

    def f1(self) -> str: ...

    class Nested2:
        def function_definition(self): ...
        assignment = 1

    def f2(self) -> str: ...

if sys.version_info > (3, 7):
    def nested1():
        assignment = 1
        def function_definition(self): ...

    def f1(self) -> str: ...
    def nested2():
        def function_definition(self): ...
        assignment = 1
    def f2(self) -> str: ...
```

This PR additionally fixes this for function definitions:

```python
if sys.version_info > (3, 7):
    if sys.platform == "win32":
        assignment = 1
        def function_definition(self): ...

    def f1(self) -> str: ...
    if sys.platform != "win32":
        def function_definition(self): ...
        assignment = 1

    def f2(self) -> str: ...

if sys.version_info > (3, 8):
    if sys.platform == "win32":
        assignment = 1
        def function_definition(self): ...

    class F1: ...
    if sys.platform != "win32":
        def function_definition(self): ...
        assignment = 1

    class F2: ...
```

You can see the effect of this change on typeshed in https://github.com/konstin/typeshed/pull/1/files. As baseline, the preview mode changes without this PR are at konstin/typeshed#2.
JelleZijlstra added a commit that referenced this issue Sep 9, 2023
The idea behind this change is that we stop looking into previous body to determine if there should be a blank before a function or class definition.

Input:

```python
import sys

if sys.version_info > (3, 7):
    class Nested1:
        assignment = 1
        def function_definition(self): ...
    def f1(self) -> str: ...
    class Nested2:
        def function_definition(self): ...
        assignment = 1
    def f2(self) -> str: ...

if sys.version_info > (3, 7):
    def nested1():
        assignment = 1
        def function_definition(self): ...
    def f1(self) -> str: ...
    def nested2():
        def function_definition(self): ...
        assignment = 1
    def f2(self) -> str: ...
```

Stable style
```python
import sys

if sys.version_info > (3, 7):
    class Nested1:
        assignment = 1
        def function_definition(self): ...

    def f1(self) -> str: ...

    class Nested2:
        def function_definition(self): ...
        assignment = 1
    def f2(self) -> str: ...

if sys.version_info > (3, 7):
    def nested1():
        assignment = 1
        def function_definition(self): ...

    def f1(self) -> str: ...
    def nested2():
        def function_definition(self): ...
        assignment = 1
    def f2(self) -> str: ...
```

In the stable formatting, we have a blank line sometimes, not depending on the previous statement on the same level, but on the last (potentially nested) statement in the previous body.

#2783/#3564 fixes this for classes in preview style:

```python
import sys

if sys.version_info > (3, 7):
    class Nested1:
        assignment = 1
        def function_definition(self): ...

    def f1(self) -> str: ...

    class Nested2:
        def function_definition(self): ...
        assignment = 1

    def f2(self) -> str: ...

if sys.version_info > (3, 7):
    def nested1():
        assignment = 1
        def function_definition(self): ...

    def f1(self) -> str: ...
    def nested2():
        def function_definition(self): ...
        assignment = 1
    def f2(self) -> str: ...
```

This PR additionally fixes this for function definitions:

```python
if sys.version_info > (3, 7):
    if sys.platform == "win32":
        assignment = 1
        def function_definition(self): ...

    def f1(self) -> str: ...
    if sys.platform != "win32":
        def function_definition(self): ...
        assignment = 1

    def f2(self) -> str: ...

if sys.version_info > (3, 8):
    if sys.platform == "win32":
        assignment = 1
        def function_definition(self): ...

    class F1: ...
    if sys.platform != "win32":
        def function_definition(self): ...
        assignment = 1

    class F2: ...
```

You can see the effect of this change on typeshed in https://github.com/konstin/typeshed/pull/1/files. As baseline, the preview mode changes without this PR are at konstin/typeshed#2.

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
F: empty lines Wasting vertical space efficiently. S: accepted The changes in this design / enhancement issue have been accepted and can be implemented T: style What do we want Blackened code to look like?
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants