Preserve indent around multiline strings #9637
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
MichaReiser
added
formatter
|
| if_group_breaks(&soft_line_break()).with_group_id(Some(indented)) | ||
| ] | ||
| ) | ||
| } |
There was a problem hiding this comment.
So this got much simpler because we no longer check if indenting would allow us to avoid line-length violations?
There was a problem hiding this comment.
Yes. Breaking only if the first line doesn't fit required a more complicated IR. Now it's simpler, because we always or never indent.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This PR changes our
multiline_stringpreview style implementation (added in #9243) to only hug strings in call arguments when there's no newline between the(and the string's quotes.This is to address the feedback raised in Black's repository (issue)
The stable formatting always indents strings:
The preview style as it is implemented today (not this PR) tries to hug multiline strings, and only falls back to indenting the string if the first line of the string exceeds the configured line width:
The benefit of the new style is that it reduces vertical spacing (The two strings in the examples aren't equivalent because of the whitespace before the closing quotes but the whitespace before the closing quotes with
indentis only used to align the quotes.The main concern with the new preview style is that it messes up the multiline string formatting for existing Black users because it can dealign the quotes and fixing the quote alignment isn't possible without knowing if the argument is whitespace sensitive or not.
For example, it turns...
into...
which looks worse.
However, the preview style improves formatting for new black users or new code written when you have:
because it no longer changes it to
This PR refiens the
multiline_stringpreview style with a heuristic of when to hug the multiline string and when not.The idea is to honor the author's decision by checking if the opening parentheses and the string both start on the same line. If so, hug the multline string, otherwise don't.
There are two advantages to this:
This heuristic is the same as Prettier uses for single-argument, multiline string call expressions.
Downsides
The downside of this heuristic over always hugging single argument multiline strings is that existing calls to
dedentneed manual updating to match the desired formatting. I think that's fine because the new formatting also requires to manually updating the spacing before the closing"""to match indentation:Doesn't look good. You want
which ruff can't do without changing the string's semantics.
Test Plan
Running Ruff before introducing the
multline-stringpreview style and then this branch:There are no upgrade changes, compared to the original
multiline-stringpreview style PR that changed+2971 -5900 lines in 374 files in 31 projects.ℹ️ ecosystem check detected format changes. (+7 -8 lines in 3 files in 3 projects; 1 project error; 39 projects unchanged)
demisto/content (+2 -5 lines across 1 file)
Packs/IntegrationsAndIncidentsHealthCheck/Scripts/GetFailedTasks/GetFailedTasks.py~L107
) if is_error(response): - error = ( - f'Failed retrieving tasks for incident ID {incident["id"]}.\n \ + error = f'Failed retrieving tasks for incident ID {incident["id"]}.\n \ Make sure that the API key configured in the Core REST API integration \ -is one with sufficient permissions to access that incident.\n' - + get_error(response) - ) +is one with sufficient permissions to access that incident.\n' + get_error(response) raise Exception(error) return response[0]["Contents"]["response"]reflex-dev/reflex (+3 -1 lines across 1 file)
reflex/components/markdown/markdown.py~L253
} # Separate out inline code and code blocks. - components["code"] = f"""{{({{node, inline, className, {_CHILDREN._var_name}, {_PROPS._var_name}}}) => {{ + components[ + "code" + ] = f"""{{({{node, inline, className, {_CHILDREN._var_name}, {_PROPS._var_name}}}) => {{ const match = (className || '').match(/language-(?<lang>.*)/); const language = match ? match[1] : ''; if (language) {{rotki/rotki (+2 -2 lines across 1 file)
rotkehlchen/rotkehlchen.py~L1196
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,openai/openai-cookbook (error)
Running ruff preview (main) and then this branch:
Results in no changes
ℹ️ ecosystem check encountered format errors. (no format changes; 1 project error)
openai/openai-cookbook (error)