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

Parenthesize singleton yield tuples #3912

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft

Conversation

spagh-eddie
Copy link
Contributor

@spagh-eddie spagh-eddie commented Oct 2, 2023

Description

Closes #3851

Please let me know if I should be going along a different path c^:

I had originally intended to make a visit_yield_expr = partial(v, keywords={"yield"}, parens={"yield"}) to be more similar to return alongside this, but that doesn't work since yield can be an expression but return can't.

self.visit_return_stmt = partial(v, keywords={"return"}, parens={"return"})

Checklist - did you ...

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

@spagh-eddie spagh-eddie changed the title Parens Parenthesize singleton yield tuples Oct 2, 2023
@github-actions
Copy link

github-actions bot commented Oct 2, 2023

diff-shades results comparing this PR (ebefed8) to main (1b08cbc). The full diff is available in the logs under the "Generate HTML diff report" step.

╭─────────────────────── Summary ────────────────────────╮
│ 3 projects & 7 files changed / 88 changes [+58/-30]    │
│                                                        │
│ ... out of 2 493 655 lines, 11 741 files & 23 projects │
╰────────────────────────────────────────────────────────╯

Differences found.

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

Copy link
Collaborator

@JelleZijlstra JelleZijlstra left a comment

Choose a reason for hiding this comment

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

I ran this diff also on our internal codebase, which very heavily uses yield because of https://github.com/quora/asynq. Most of the changes are clear improvements, but I noticed a few cases like this:

-        content_type = yield (
-            a.model.tribe.item.TribeItem(tribe_item_id).get_content_type.asynq()
-        )
+        content_type = yield a.model.tribe.item.TribeItem(
+            tribe_item_id
+        ).get_content_type.asynq()

Here the old formatting seems better.

There are also cases like this:

                 cant_edit = (
-                    yield a.controller.tribe.item.is_tribe_answer_and_cant_edit.asynq(
-                        self.aid, self.viewer
+                    yield (
+                        a.controller.tribe.item.is_tribe_answer_and_cant_edit.asynq(
+                            self.aid, self.viewer
+                        )
                     )
                 )

Where ideally we should remove the outer set of parentheses.

Linking #1553, another issue with how we manage yields; it may make sense to tackle them together.

@@ -11,6 +11,8 @@
<!-- Changes that affect Black's stable style -->

- Fix comments getting removed from inside parenthesized strings (#3909)
- Parenthesize singleton tuples in `yield` expressions as they already are in `return`
Copy link
Collaborator

Choose a reason for hiding this comment

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

This needs to go in the preview style, not the stable style. (That's also why CI is failing.)

@spagh-eddie spagh-eddie marked this pull request as draft October 2, 2023 17:58
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.

single element tuples in yield expressions are not parenthesized
2 participants