Skip to content

Remove trailing whitespace in multi-line string literals of tests #1322

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 1 commit into from
Feb 9, 2023

Conversation

ahoppen
Copy link
Member

@ahoppen ahoppen commented Feb 7, 2023

In mulit-line string literal tests these trailing whitespaces are really hard to spot but influence the test significantly. To make them more obvious, use \u{20}, which is visible in the editor and will be replaced by a normal space in AssertParse

In all other cases where the trailing whitespace doesn’t matter, remove it.

@ahoppen ahoppen requested a review from bnbarham February 7, 2023 11:20
@ahoppen
Copy link
Member Author

ahoppen commented Feb 7, 2023

@swift-ci Please test

@bnbarham
Copy link
Contributor

bnbarham commented Feb 7, 2023

How many tests actually need this? Rintaro suggested that we could just have this inline on each of the tests instead. I'd also prefer it not be replaced with anything (ie. replace with empty string) and just be a marker for "keep this whitespace".

@allevato
Copy link
Member

allevato commented Feb 7, 2023

FWIW, in swift-format we just write literal \u{0020} in any strings where we want to trailing space to be explicit, visible, and not removed automatically by tooling, and that doesn't require any custom logic for replacement: https://github.com/apple/swift-format/blob/3cd655ff323bd22be523dd33b5e1cd0b14f6bf0a/Tests/SwiftFormatPrettyPrintTests/CommentTests.swift#L651

In mulit-line string literal tests these trailing whitespaces are really hard to spot but influence the test significantly. To make them more obvious, use `\u{20}`, which is visible in the editor and will be replaced by a normal space in `AssertParse`

In all other cases where the trailing whitespace doesn’t matter, remove it.
@ahoppen ahoppen force-pushed the ahoppen/no-trailing-whitespace branch from f8cf3d5 to fbecd19 Compare February 8, 2023 07:59
@ahoppen
Copy link
Member Author

ahoppen commented Feb 8, 2023

I like the idea of using \u{20} and adopted that.

@ahoppen
Copy link
Member Author

ahoppen commented Feb 8, 2023

@swift-ci Please test

1 similar comment
@ahoppen
Copy link
Member Author

ahoppen commented Feb 8, 2023

@swift-ci Please test

Copy link
Contributor

@bnbarham bnbarham left a comment

Choose a reason for hiding this comment

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

The commit comment doesn't really apply now, but looks good otherwise.

which is visible in the editor and will be replaced by a normal space in AssertParse

It's not really "replaced" by AssertParse any more - just by the compiler 😅.

@ahoppen
Copy link
Member Author

ahoppen commented Feb 8, 2023

Oh, I changed the commit message, just forgot to update the PR description.

@ahoppen
Copy link
Member Author

ahoppen commented Feb 8, 2023

@swift-ci Please test macOS

@bnbarham
Copy link
Contributor

bnbarham commented Feb 8, 2023

Oh, I changed the commit message, just forgot to update the PR description.

I was referring to the commit message as well, FWIW.

@ahoppen ahoppen merged commit eb16cd3 into swiftlang:main Feb 9, 2023
@ahoppen ahoppen deleted the ahoppen/no-trailing-whitespace branch February 9, 2023 22:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants