Fix instability with await fluent style by konstin · Pull Request #8676 · astral-sh/ruff

konstin · 2023-11-14T14:33:13Z

Fix an instability where await was followed by a breaking fluent style expression:

test_data = await (
    Stream.from_async(async_data)
    .flat_map_async()
    .map()
    .filter_async(is_valid_data)
    .to_list()
)

Note that this technically a minor style change (see ecosystem check)

konstin · 2023-11-14T14:33:31Z

...thon_formatter/tests/snapshots/black_compatibility@simple_cases__remove_await_parens.py.snap

+ async def main():
+-    await asyncio.sleep(1)  # Hello
+    await (
+        asyncio.sleep(1)  # Hello
+    )


This is arguably correct for ruff

This seems like really weird formatting. Why is this correct?

Note that we don't expand, we just keep

async def main(): await asyncio.sleep(1) # Hello await ( asyncio.sleep(1) # Hello )

as-is

Okay that's good to know. I guess that's okay. A little weird but fits with previous choices?

Why does this work with IfBreaks?

In yield we use Parenthesize::Optional...

I tried that too for that same reason, but it introduced new black deviations:

# Remove brackets for short coroutine/task async def main(): - await asyncio.sleep(1) + await (asyncio.sleep(1)) async def main(): - await asyncio.sleep(1) + await (asyncio.sleep(1)) async def main(): - await asyncio.sleep(1) + await (asyncio.sleep(1))

github-actions · 2023-11-14T14:45:05Z

`ruff-ecosystem` results

Formatter (stable)

ℹ️ ecosystem check detected format changes. (+9 -5 lines in 1 file in 41 projects)

RasaHQ/rasa (+9 -5 lines across 1 file)

rasa/server.py~L880

 
         try:
             async with app.ctx.agent.lock_store.lock(conversation_id):
-                tracker = await app.ctx.agent.processor.fetch_tracker_and_update_session(
-                    conversation_id
+                tracker = await (
+                    app.ctx.agent.processor.fetch_tracker_and_update_session(
+                        conversation_id
+                    )
                 )
 
                 output_channel = _get_output_channel(request, tracker)

rasa/server.py~L931

 
         try:
             async with app.ctx.agent.lock_store.lock(conversation_id):
-                tracker = await app.ctx.agent.processor.fetch_tracker_and_update_session(
-                    conversation_id
+                tracker = await (
+                    app.ctx.agent.processor.fetch_tracker_and_update_session(
+                        conversation_id
+                    )
                 )
                 output_channel = _get_output_channel(request, tracker)
                 if intent_to_trigger not in app.ctx.agent.domain.intents:

Formatter (preview)

ℹ️ ecosystem check detected format changes. (+9 -5 lines in 1 file in 41 projects)

RasaHQ/rasa (+9 -5 lines across 1 file)

ruff format --preview

rasa/server.py~L880

 
         try:
             async with app.ctx.agent.lock_store.lock(conversation_id):
-                tracker = await app.ctx.agent.processor.fetch_tracker_and_update_session(
-                    conversation_id
+                tracker = await (
+                    app.ctx.agent.processor.fetch_tracker_and_update_session(
+                        conversation_id
+                    )
                 )
 
                 output_channel = _get_output_channel(request, tracker)

rasa/server.py~L931

 
         try:
             async with app.ctx.agent.lock_store.lock(conversation_id):
-                tracker = await app.ctx.agent.processor.fetch_tracker_and_update_session(
-                    conversation_id
+                tracker = await (
+                    app.ctx.agent.processor.fetch_tracker_and_update_session(
+                        conversation_id
+                    )
                 )
                 output_channel = _get_output_channel(request, tracker)
                 if intent_to_trigger not in app.ctx.agent.domain.intents:

charliermarsh · 2023-11-14T15:50:21Z

crates/ruff_python_formatter/src/expression/expr_await.rs

                token("await"),
                space(),
-                maybe_parenthesize_expression(value, item, Parenthesize::IfRequired)
+                maybe_parenthesize_expression(value, item, Parenthesize::IfBreaks)


Should this be IfBreaksOrIfRequired?

That one would regress asyncio.gather

# Long lines async def main(): - await asyncio.gather( - asyncio.sleep(1), - asyncio.sleep(1), - asyncio.sleep(1), - asyncio.sleep(1), - asyncio.sleep(1), - asyncio.sleep(1), - asyncio.sleep(1), + await ( + asyncio.gather( + asyncio.sleep(1), + asyncio.sleep(1), + asyncio.sleep(1), + asyncio.sleep(1), + asyncio.sleep(1), + asyncio.sleep(1), + asyncio.sleep(1), + ) )

(i'm happy too implement a different solution btw, this was just the solution that i found to work best by trial and error)

Ahh yeah, I agree that that's worse. I feel like I need to have a better understanding of why these Parenthesize values are leading to these changes before I'd be comfortable approving. Why does IfBreaks break that way? Will using IfBreaks ever lead to syntax errors?

No, that's handled by OptionalParentheses::Multiline

I'll make some better examples why we get this behavior

Thank you, I'm trying to make sure I play Micha and really understand the impact of formatting changes before approving.

From the perspective of the await, it needs OptionalParentheses::Never because the child is already parenthesized, and we're preferring those parentheses over adding them around the await. For the await's child, we have four options:

Optional: Add parentheses if the child breaks, which we want e.g. for asyncio.gather. Keeps the child's parentheses from the source code, which we want to remove.

IfBreaks: Add parentheses if the child breaks, which we want e.g. for asyncio.gather. Discards child parentheses, which we want.

IfRequired: Discards child parentheses, which we want. Also discards them when the child breaks but has its own inner parentheses, which would lead to e.g.
await asyncio.gather( fut1, fut2, )
, which don't want.

IfBreaksOrIfRequired: Special case for return type annotations.

I added comments to the parentheses types to make them better understandable in the future

Fix instability with await fluent style

2191dd8

konstin added the formatter Related to the formatter label Nov 14, 2023

konstin marked this pull request as ready for review November 14, 2023 14:33

konstin commented Nov 14, 2023

View reviewed changes

charliermarsh reviewed Nov 14, 2023

View reviewed changes

Add comments about parentheses types

a9fbaaa

konstin requested a review from charliermarsh November 17, 2023 09:07

charliermarsh approved these changes Nov 17, 2023

View reviewed changes

charliermarsh merged commit dca430f into main Nov 17, 2023

charliermarsh deleted the fix-8644 branch November 17, 2023 17:24

charliermarsh mentioned this pull request Nov 17, 2023

Formatting instability in async call chain #8644

Closed

miccal mentioned this pull request Nov 17, 2023

ruff 0.1.6 Homebrew/homebrew-core#154716

Merged

Conversation

konstin commented Nov 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Nov 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

ruff-ecosystem results

Formatter (stable)

Formatter (preview)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

konstin commented Nov 14, 2023 •

edited

Loading

github-actions bot commented Nov 14, 2023 •

edited

Loading

`ruff-ecosystem` results