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

More concise formatting for dummy implementations #3796

Merged
merged 9 commits into from
Aug 4, 2023

Conversation

hauntsaninja
Copy link
Collaborator

@hauntsaninja hauntsaninja commented Jul 17, 2023

Fixes #1797, in particular, see #1797 (comment)

@github-actions

This comment was marked as outdated.

@ichard26
Copy link
Collaborator

Addressed by #3799.

@ichard26 ichard26 closed this Jul 18, 2023
@hauntsaninja
Copy link
Collaborator Author

Thanks for improving the comment! This is my testing PR, was looking at #1797 (comment) and wanted to see what diff-shades has to say about potential changes :-)

@hauntsaninja hauntsaninja reopened this Jul 18, 2023
@ichard26
Copy link
Collaborator

Ah I didn't realize that, haha. I thought you were wondering what was up with the comment looking wonky :P

@hauntsaninja hauntsaninja changed the title [ignore] curious for diff shade output More concise formatting for dummy implementations Jul 22, 2023
@hauntsaninja hauntsaninja marked this pull request as ready for review July 22, 2023 20:50
@@ -165,6 +165,13 @@ def is_def(self) -> bool:
and second_leaf.value == "def"
)

@property
def is_stub_def(self) -> bool:
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Mirrors is_stub_class from a couple lines above

and self.previous_line.is_stub_def
and (current_line.is_stub_def or current_line.is_decorator)
):
newlines = user_hint_before
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is perhaps controversial, but it allows users to group things as they want (e.g. in the test case, dummy and other are grouped separately) and solves some lookahead problems when dealing with decorators

src/black/lines.py Outdated Show resolved Hide resolved
@property
def is_stub_def(self) -> bool:
"""Is this line a function definition with a body consisting only of "..."?"""
return self.is_def and self.leaves[-3:] == [
Copy link
Collaborator

Choose a reason for hiding this comment

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

Doesn't this also match something like def f(x): y = ...?

Copy link
Collaborator Author

@hauntsaninja hauntsaninja Jul 23, 2023

Choose a reason for hiding this comment

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

Yes, this is true (and true for the is is_stub_class heuristic above as well). I've made the heuristic slightly better by including the colon. I didn't see anything adverse in the diff shades output.

Also note some of the pyi code we now use in linegen.py will trigger in some cases involving other statements, although it's not unreasonable when it happens.

tests/data/preview/dummy_implementations.py Show resolved Hide resolved
@hauntsaninja
Copy link
Collaborator Author

hauntsaninja commented Jul 23, 2023

I didn't like the way my previous implementation special cased decorators, so I've taken a more normalised approach in this latest commit.

I could still check for current_line.is_stub_def and forcibly remove newlines... this would make Black remove several newlines in Protocols, but wouldn't help with overloads / decorators. I chose not to do this for consistency with pyi mode, where we allow users to insert newlines if they feel like.

@hauntsaninja hauntsaninja merged commit c160e4b into psf:main Aug 4, 2023
29 checks passed
@JelleZijlstra
Copy link
Collaborator

This change isn't in any release, and is currently only in the preview mode. If you don't want such new changes, use the stable style. Also, if you are using Black to format your code, probably best to not also run the PyCharm formatter, as we don't generally attempt to keep Black compatible with it.

@hauntsaninja hauntsaninja deleted the see-diff-shades branch August 19, 2023 00:49
copybara-service bot pushed a commit to google/pyink that referenced this pull request Oct 17, 2023
Notably the the preview style for "dummy implementations" is disabled in Pyink as it doesn't play well with our linter (yet). The change in _Black_ is from psf#3796.

PiperOrigin-RevId: 573023762
copybara-service bot pushed a commit to google/pyink that referenced this pull request Oct 17, 2023
Notably the the preview style for "dummy implementations" is disabled in Pyink as it doesn't play well with our linter (yet). The change in _Black_ is from psf#3796.

PiperOrigin-RevId: 573023762
copybara-service bot pushed a commit to google/pyink that referenced this pull request Oct 17, 2023
Notably the the preview style for "dummy implementations" is disabled in Pyink as it doesn't play well with our linter (yet). The change in _Black_ is from psf#3796.

PiperOrigin-RevId: 573023762
copybara-service bot pushed a commit to google/pyink that referenced this pull request Oct 17, 2023
Notably the the preview style for "dummy implementations" is disabled in Pyink as it doesn't play well with our linter (yet). The change in _Black_ is from psf#3796.

PiperOrigin-RevId: 573023762
copybara-service bot pushed a commit to google/pyink that referenced this pull request Oct 17, 2023
Notably the the preview style for "dummy implementations" is disabled in Pyink as it doesn't play well with our linter (yet). The change in _Black_ is from psf#3796.

PiperOrigin-RevId: 573023762
copybara-service bot pushed a commit to google/pyink that referenced this pull request Oct 17, 2023
Notably the the preview style for "dummy implementations" is disabled in Pyink as it doesn't play well with our linter (yet). The change in _Black_ is from psf#3796.

PiperOrigin-RevId: 573023762
copybara-service bot pushed a commit to google/pyink that referenced this pull request Oct 17, 2023
Notably the the preview style for "dummy implementations" is disabled in Pyink as it doesn't play well with our linter (yet). The change in _Black_ is from psf#3796.

PiperOrigin-RevId: 574275989
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.

Allow reduced number of blank lines for dummy implementations
3 participants