-
Notifications
You must be signed in to change notification settings - Fork 230
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
Add a configuration to preserve whitespace on lines containing only whitespace #804
Conversation
if configuration.allowWhitespaceOnlyLines, outputBuffer.isAtStartOfLine { | ||
let currentIndentationSpaceSize = outputBuffer.currentIndentation.indentation().count | ||
outputBuffer.write(String(repeating: " ", count: size - currentIndentationSpaceSize)) | ||
} else { |
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 current implementation preserves whitespace exactly as it is.
Input (Output is the same as the input)
class A {
func foo() -> Int {
return 1
}
// 5 spaces
func bar() -> Int {
return 2
}
}
Would it be better to modify it so that only the current level of indentation is preserved, like this?
if configuration.allowWhitespaceOnlyLines, outputBuffer.isAtStartOfLine { | |
let currentIndentationSpaceSize = outputBuffer.currentIndentation.indentation().count | |
outputBuffer.write(String(repeating: " ", count: size - currentIndentationSpaceSize)) | |
} else { | |
if configuration.allowWhitespaceOnlyLines, outputBuffer.isAtStartOfLine { | |
let currentIndentationSpaceSize = outputBuffer.currentIndentation.indentation().count | |
outputBuffer.write(String()) | |
} else { |
Output will be changed to
class A {
func foo() -> Int {
return 1
}
// 2 spaces(same as indentation)
func bar() -> Int {
return 2
}
}
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 people want this to match the behavior in Xcode (which I personally think is undesirable, but...), then the number of spaces should be changed to match the expected indentation if someone were to type on that line.
For example, if I have this:
func f() {}
// <- 5 spaces
func g() {}
Then I wrap it in a struct
, once I type the closing }
, Xcode will re-indent the whole thing. When that happens, the line has changed to this:
struct S {
func f() {}
// <- 2 spaces
func g() {}
}
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.
To completely match the behavior with Xcode, we need to create spaces according to the indentation even when there are no existing spaces. For now, I've modified it so that the indentation is adjusted only when spaces are present.
Do you think we need to generate spaces to match the indentation even when there are no spaces?
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.
It looks like Xcode does insert spaces there, even when there aren't any.
@ahoppen @bnbarham Do you all have any strong opinions here? I've never liked this behavior in Xcode anyway (I always tell it to strip whitespace-only lines) but I could see myself arguing for either direction here. (And I'd rather not make it another separate knob on top of the one already being added 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 too would prefer not having an additional option for this, but I can definitely see some people wanting "ident all the things including blank lines" and others wanting "keep my trailing whitespace but don't add indentation to blank lines". So I'm not sure there's a good way around it :(.
Though TBH I don't fully understand the use case for either of these - might ask that in the issue, they do say "this is my favorite feature".
If we do just have the one, rather than allowWhitespaceOnlyLines
maybe indentBlankLines
/formatBlankLines
is clearer?
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.
That sounds good to me!
Once we've finalized the implementation direction from the ongoing discussion, I'll update the configuration name as you suggested to make it clearer.
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 renamed the configuration to indentBlankLines
for better clarity and updated the comments accordingly.
Additionally, I modified it so that when indentBlankLines
is set to true, it adds spaces that match the current indent, even if there are no spaces in the blank lines.
input
struct S {
func f() {}
// <- no space
func g() {}
}
output
struct S {
func f() {}
// <- 2 spaces
func g() {}
}
final class AllowWhitespaceOnlyLinesTests: PrettyPrintTestCase { | ||
func testAllowWhitespaceOnlyLinesEnabled() { | ||
let input = | ||
""" |
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.
In this file, can you replace any trailing whitespace with literal \u{0020}
escape sequences? That makes it clear to the reader what's being tested, and we don't risk the spaces being lost if someone's editor strips trailing spaces.
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.
Oh, that part was exactly what I was struggling with, and I didn’t know there was such a great solution! Thank you!
44f83b4
to
162a271
Compare
@@ -449,7 +449,11 @@ public class PrettyPrinter { | |||
|
|||
// Print out the number of spaces according to the size, and adjust spaceRemaining. | |||
case .space(let size, _): | |||
outputBuffer.enqueueSpaces(size) | |||
if configuration.allowWhitespaceOnlyLines, outputBuffer.isAtStartOfLine { | |||
outputBuffer.write(String()) |
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.
Can you leave a brief comment here explaining why we do this? To a random observer, asking it to write an empty string feels odd, but it's because we need the buffer to still do its other logic (flushing the pending indentation, etc.).
I'd also just write it as outputBuffer.write("")
; same effect but easier to read/scan.
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 a comment and made the modification!
162a271
to
ecdd9f7
Compare
ecdd9f7
to
071407e
Compare
Really looking forward to this! |
leadingIndent = .spaces(n) | ||
if config.indentBlankLines, trivia.count > index + 1, trivia[index + 1].isNewline { | ||
appendToken(.space(size: n)) | ||
requiresNextNewline = true |
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.
Does this mean that superfluous newline are no longer discarded if config.indentBlankLines
is true? That seems surprising to me. Eg.
func foo() {}
func bar() {}
should still be formatted to only have one newline.
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.
Looking at it again, this logic doesn’t seem appropriate 🤔
In the initial implementation, this was added along with logic to retain user-entered spaces by inserting a space
token, but it doesn’t seem necessary with the new implementation approach.
I’ll keep the logic that adds spaces as leadingIndent
and, when processing newlines, add a soft newline if there’s a leadingIndent
, ensuring that the actual output doesn’t exceed the configured limit lines.
ec97f7b
to
6d5ec24
Compare
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.
Thanks. Just two minor comments, otherwise LGTM.
6d5ec24
to
f3ccba9
Compare
I've applied all the comments you suggested. Thank you for the kind review 🙇♂️ |
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.
Thank you for implementing this, @TTOzzi. Looks good.
Head branch was pushed to by a user without write access
f3ccba9
to
3065950
Compare
Oops, sorry. The format check failed 😓 |
No worries. Already re-triggered CI. |
Oh no, it failed on a different file this time 😨 |
You should be able to just cd to the swift-format directory and then run |
Head branch was pushed to by a user without write access
3065950
to
8c68ec3
Compare
I'm done! Thank you! |
Resolve #801
I've implemented an option to preserve whitespace on lines that consist solely of whitespace.
I believe this option will be useful for many people, but I'm not sure if I've implemented it in the correct way 🤔
If the implementation direction is not correct, please feel free to let me know. Thank you 🙇