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

Fix unstable with-items formatting #10274

Merged
merged 1 commit into from
Mar 8, 2024
Merged

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
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
17 checks passed
@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