Skip to content

Fix unstable with-items formatting#10274

Merged
charliermarsh merged 1 commit intomainfrom
fix-with-item-parenthesizing
Mar 8, 2024
Merged

Fix unstable with-items formatting#10274
charliermarsh merged 1 commit intomainfrom
fix-with-item-parenthesizing

Conversation

@MichaReiser
Copy link
Member

@MichaReiser MichaReiser commented Mar 7, 2024

Summary

Fixes #10267

The issue with the current formatting is that the formatter flips between the SingleParenthesizedContextManager and ParenthesizeIfExpands or SingleWithTarget because the layouts use incompatible formatting ( SingleParenthesizedContextManager: maybe_parenthesize_expression(context) vs ParenthesizeIfExpands: parenthesize_if_expands(item), SingleWithTarget: optional_parentheses(item).

The fix is to ensure that the layouts between which the formatter flips when adding or removing parentheses are the same. I do this by introducing a new SingleWithoutTarget layout that uses the same formatting as SingleParenthesizedContextManager if it has no target and prefer SingleWithoutTarget over using ParenthesizeIfExpands or SingleWithTarget.

Formatting change

The downside is that we now use maybe_parenthesize_expression over parenthesize_if_expands for expressions where can_omit_optional_parentheses returns false. This can lead to stable formatting changes. I only found one formatting change in our ecosystem check and, unfortunately, this is necessary to fix the instability (and instability fixes are okay to have as part of minor changes according to our versioning policy)

The benefit of the change is that with items with a single context manager and without a target are now formatted identically to how the same expression would be formatted in other clause headers.

Test Plan

I ran the ecosystem check locally

@MichaReiser MichaReiser force-pushed the fix-with-item-parenthesizing branch from 2891e0a to 6b290c0 Compare March 7, 2024 15:21
@MichaReiser MichaReiser added bug Something isn't working formatter Related to the formatter labels Mar 7, 2024
@github-actions
Copy link
Contributor

github-actions bot commented Mar 7, 2024

ruff-ecosystem results

Formatter (stable)

ℹ️ ecosystem check detected format changes. (+2 -2 lines in 1 file in 1 projects; 42 projects unchanged)

rotki/rotki (+2 -2 lines across 1 file)

rotkehlchen/rotkehlchen.py~L1213

         if oracle != HistoricalPriceOracle.CRYPTOCOMPARE:
             return  # only for cryptocompare for now
 
-        with (
-            contextlib.suppress(UnknownAsset)
+        with contextlib.suppress(
+            UnknownAsset
         ):  # if suppress -> assets are not crypto or fiat, so we can't query cryptocompare  # noqa: E501
             self.cryptocompare.create_cache(
                 from_asset=from_asset,

Formatter (preview)

ℹ️ ecosystem check detected format changes. (+2 -2 lines in 1 file in 1 projects; 42 projects unchanged)

rotki/rotki (+2 -2 lines across 1 file)

ruff format --preview

rotkehlchen/rotkehlchen.py~L1211

         if oracle != HistoricalPriceOracle.CRYPTOCOMPARE:
             return  # only for cryptocompare for now
 
-        with (
-            contextlib.suppress(UnknownAsset)
+        with contextlib.suppress(
+            UnknownAsset
         ):  # if suppress -> assets are not crypto or fiat, so we can't query cryptocompare  # noqa: E501
             self.cryptocompare.create_cache(
                 from_asset=from_asset,

@aneeshusa
Copy link

I'm traveling today but am happy to give this a try tomorrow! Amazed how quick you put this together, thank you!

@MichaReiser MichaReiser force-pushed the fix-with-item-parenthesizing branch from 6b290c0 to 25ada0a Compare March 7, 2024 16:43
@charliermarsh
Copy link
Member

Thanks @aneeshusa 👋

@MichaReiser MichaReiser force-pushed the fix-with-item-parenthesizing branch from 25ada0a to a069277 Compare March 8, 2024 11:14
@MichaReiser MichaReiser changed the base branch from main to refactor-with-item-formatting March 8, 2024 11:15
@MichaReiser MichaReiser force-pushed the fix-with-item-parenthesizing branch 2 times, most recently from 4100b9f to a42c365 Compare March 8, 2024 11:30
@MichaReiser MichaReiser marked this pull request as ready for review March 8, 2024 11:33
Copy link

@aneeshusa aneeshusa left a comment

Choose a reason for hiding this comment

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

Can't do a GitHub approval but tested this and this fixes both an isolated repro and the monorepo at $WORK! Would love to see this in a release.

Base automatically changed from refactor-with-item-formatting to main March 8, 2024 23:40
@charliermarsh charliermarsh force-pushed the fix-with-item-parenthesizing branch from a42c365 to f9416ea Compare March 8, 2024 23:41
@charliermarsh charliermarsh enabled auto-merge (squash) March 8, 2024 23:42
@charliermarsh charliermarsh merged commit 4bce801 into main Mar 8, 2024
@charliermarsh charliermarsh deleted the fix-with-item-parenthesizing branch March 8, 2024 23:48
@charliermarsh
Copy link
Member

Just shipped this in v0.3.2 :)

nkxxll pushed a commit to nkxxll/ruff that referenced this pull request Mar 10, 2024
## Summary

Fixes astral-sh#10267

The issue with the current formatting is that the formatter flips
between the `SingleParenthesizedContextManager` and
`ParenthesizeIfExpands` or `SingleWithTarget` because the layouts use
incompatible formatting ( `SingleParenthesizedContextManager`:
`maybe_parenthesize_expression(context)` vs `ParenthesizeIfExpands`:
`parenthesize_if_expands(item)`, `SingleWithTarget`:
`optional_parentheses(item)`.

The fix is to ensure that the layouts between which the formatter flips
when adding or removing parentheses are the same. I do this by
introducing a new `SingleWithoutTarget` layout that uses the same
formatting as `SingleParenthesizedContextManager` if it has no target
and prefer `SingleWithoutTarget` over using `ParenthesizeIfExpands` or
`SingleWithTarget`.

## Formatting change

The downside is that we now use `maybe_parenthesize_expression` over
`parenthesize_if_expands` for expressions where
`can_omit_optional_parentheses` returns `false`. This can lead to stable
formatting changes. I only found one formatting change in our ecosystem
check and, unfortunately, this is necessary to fix the instability (and
instability fixes are okay to have as part of minor changes according to
our versioning policy)

The benefit of the change is that `with` items with a single context
manager and without a target are now formatted identically to how the
same expression would be formatted in other clause headers.

## Test Plan

I ran the ecosystem check locally
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working formatter Related to the formatter

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Ruff has unstable results with target-version (normally hidden by caching)

3 participants