-
Notifications
You must be signed in to change notification settings - Fork 246
Wrap multiline string literals to line length. #792
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
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since backslashes are used to suppress newlines, I think I agree with this choice. I'll admit it's somewhat opinionated though. I don't know if other folks would want to be able to choose where in a string they break it. If we made this configurable, we could allow that, but the problem with doing that is that once a string is broken in this way, it can never be automatically unbroken, so shifting indentation could cause a string to split awkwardly.
@ahoppen @bnbarham Do either of you have opinions here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I personally would not automatically re-flow string literals. Most likely a user already put thought into the re-flow. If you do want to re-flow, I personally think it’s not too complicated to change the string literal to be on a single line again and then let swift-format break it again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The main case that this isn't true is what @allevato mentioned, ie. format causes a split, now you indent for some other reason (nest in an if/whatever), and now never automatically reflow.
Could have a refactoring for this pretty easily too.
I'm somewhat split in my opinion as I can see benefits in both. I definitely think we need to have an option (probably defaulted to off) if we do reflow though. Maybe
// swift-format-ignore
is enough for cases when it's on?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added an option to configure multiline string reflow behavior. When false, existing escaped newlines are left in place and string reflow does not occur. When true, existing escaped newlines are removed and string reflows using the inserted
.escaped
break tokens.The option defaults to false, so reflow will not occur by default and will be opt-in.
// swift-format-ignore
works regardless of this setting, so folks can turn on reflow and still keep escaped newlines for specific literals.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spoke with @allevato and extended the reflow config behavior to include 3 options:
never
,onlyLinesOverLength
, andalways
.never
behaves the same as today, we never break up multiline strings.onlyLinesOverLength
respects existing escaped line breaks but will break up lines over the line length.always
does not respect existing escaped line breaks. It will remove existing escaped line breaks and reflow everything.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After chatting offline, we decided on the above and a couple other changes:
never
, to preserve existing behavior (don't break or reflow strings)onlyLinesOverLength
oralways
, only do it for regular multiline strings ("""
), never for raw multiline strings (#"""
, etc.)The rationale for (2) is that it's highly likely that raw multiline strings are used for things like structured data, code, etc., whereas regular multiline strings are more likely to be simple prose.
That's not always the case, of course; swift-syntax and swift-format both have a mixture of code snippets in regular and raw multiline strings. But if we wanted to change the default behavior for this later, we could update those so that they format correctly in each case.
@ahoppen @bnbarham Do you think this is a reasonable landing spot for the time being?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems reasonable to me! Also CC @shawnhyam