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

Add support to style function definitions with newlines before function stubs #4318

Merged

Conversation

peterkra25
Copy link
Contributor

Description

Per Issue #4291, Black previously failed to format function definitions that contained an empty line before an indented function stub. This change resolves this bug by removing any newlines in the prefix of the first dot in a function stub after a stub suite is found. We added this change in the visit_default() function of linegen.py.

Through this change, functions defined as below:

class C:
    def f(self):

        ...

are formatted as:

class C:
    def f(self): ...

Checklist - did you ...

  • [Y] Add an entry in CHANGES.md if necessary?
  • [Y] Add / update tests if necessary?
  • [Y] Add new / update outdated documentation?

This change was added by Peter Kraszulyak and Harvin Mumick.

Copy link

github-actions bot commented Apr 22, 2024

diff-shades reports zero changes comparing this PR (8dc6217) to main (f4b644b).


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

@peterkra25
Copy link
Contributor Author

We are unsure as to why the “Lint + format ourselves / build” test fails on flake8 when running pre-commit. After first cloning the repo and before making any changes, we ran

pre-commit run -a

and it reported that flake8 failed. This message persisted after we made our changes, so we restarted by deleting our project folder and cloning the entire repo again, but we still received the same error. If anybody knows what may be causing flake8 to fail, please let us know!

@hauntsaninja
Copy link
Collaborator

It looks like flake8-bugbear made a new release including a new check that has some false positives. I filed PyCQA/flake8-bugbear#467 , but we should just pin bugbear or maybe disable that error code

hauntsaninja added a commit to hauntsaninja/black that referenced this pull request Apr 22, 2024
hauntsaninja added a commit that referenced this pull request Apr 22, 2024
Copy link
Collaborator

@hauntsaninja hauntsaninja left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! It's a little scary to me to be changing visit_default. Can we change it in a more targeted way (e.g. specifically at the call sites in #3796 when we know we're dealing with a stub body)?

As it stands, I think this will change formatting in other situations as well, for instance, if there are real statements after ....

One nit: let's move the test cases into tests/data/cases/dummy_implementations.py?

@@ -0,0 +1,7 @@
class C:
def f(self):

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe add test cases (in the same file) where there are comments in various places around the ..., e.g. on a line by itself or right after :.

@harvinmumick
Copy link

Thanks for the PR! It's a little scary to me to be changing visit_default. Can we change it in a more targeted way (e.g. specifically at the call sites in #3796 when we know we're dealing with a stub body)?

As it stands, I think this will change formatting in other situations as well, for instance, if there are real statements after ....

One nit: let's move the test cases into tests/data/cases/dummy_implementations.py?

We are struggling to find a more targeted way to modify the prefix at the call site. The only call site at which we know we are dealing with a stub body in the provided test cases is here:

black/src/black/linegen.py

Lines 317 to 320 in 40458d7

else:
if not node.parent or not is_stub_suite(node.parent):
yield from self.line()
yield from self.visit_default(node)

This call site verifies a stub suite when visiting the "suite" node in the syntax tree, which is 2 levels higher than the node containing the first DOT token – the node whose prefix needs to be stripped of newlines. Between this call site and the point at which we visit the first DOT token in visit_default, the suite's prefix is stripped of all indentations, which enables further stripping of newline characters by the time at which the first DOT token is visited.

Since the indents are not cleaned at the call site, we don't see a way to clean the prefix at the call site.

@hauntsaninja
Copy link
Collaborator

Why can't we do something like this?

diff --git a/src/black/linegen.py b/src/black/linegen.py
index 4b29a04..f80140b 100644
--- a/src/black/linegen.py
+++ b/src/black/linegen.py
@@ -308,8 +308,11 @@ class LineGenerator(Visitor[Line]):
                 yield from self.line(-1)
 
         else:
-            if not node.parent or not is_stub_suite(node.parent):
-                yield from self.line()
+            if node.parent and is_stub_suite(node.parent):
+                node.prefix = ""
+                yield from self.visit_default(node)
+                return
+            yield from self.line()
             yield from self.visit_default(node)
 
     def visit_async_stmt(self, node: Node) -> Iterator[Line]:

Note Node.prefix has a setter that will set its children[0]'s prefix

@peterkra25
Copy link
Contributor Author

peterkra25 commented Apr 23, 2024

Upon further review, that change works for the code snippet reported in #4291. However, for a test as @JelleZijlstra recommended containing a comment on the line immediately after the function stub such as below:

class Class:
    def f(self):

        ...
        # Comment 3

The newline in between the function definition and stub is not removed and the file is left unchanged. In our previous implementation, the newline is stripped for this case, although the stub is not folded onto the same line as the function definition when possible (i.e: when there are no comments on the same line as the function definition).

Note: both implementations work as expected when there are real statements immediately following the function stub.

@JelleZijlstra
Copy link
Collaborator

It's fine to leave that case unchanged; in fact, per our stability policy, we must maintain the current formatting for the rest of the year.

I'm asking for those additional tests mostly because comments in unusual places tend to trigger bugs in Black. I want to make sure:

  • Black doesn't crash on any inputs
  • Black doesn't remove any comments
  • Black preserves its existing formatting for any code that is currently formatted successfully (following the stability policy)

@harvinmumick
Copy link

Thank you both for your feedback and clarifications. We've relocated the implementation and added the extra tests to tests/data/cases/dummy_implementations.py

Copy link
Collaborator

@hauntsaninja hauntsaninja left a comment

Choose a reason for hiding this comment

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

Thank you for the fix!

@hauntsaninja hauntsaninja merged commit 1354be2 into psf:main Apr 24, 2024
46 checks passed
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.

4 participants