Skip to content

Remove redundant leading/trailing trivia from StringLiteralExpr #135

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Jul 24, 2019
Merged

Remove redundant leading/trailing trivia from StringLiteralExpr #135

merged 2 commits into from
Jul 24, 2019

Conversation

kitasuke
Copy link
Contributor

@kitasuke kitasuke commented Jul 22, 2019 •
edited
Loading

Overview

This PR introduced a bug to set leading/trailing trivia to StringSegment as well. This causes unexpected behavior like below

SyntaxFactory.makeStringLiteralExpr(
    "foo", 
    leadingTrivia: .newlines(1), 
    trailingTrivia: .newlines(1)
)

Expected


"foo"

Actual


"
foo
"

@kitasuke kitasuke mentioned this pull request Jul 22, 2019
Closed
akyrtzi
akyrtzi requested changes Jul 22, 2019
View reviewed changes
Copy link
Contributor

@akyrtzi akyrtzi left a comment

Choose a reason for hiding this comment

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

Could you add a test case for this? Otherwise LGTM!

@kitasuke
Copy link
Contributor Author

kitasuke commented Jul 23, 2019 •
edited
Loading

Yes. I feel current SyntaxFactoryTests doesn't have enough test cases. I guess it would be ideal to have one test case for one factory method for syntax. Is this something you want me to do in this PR, or should I just add minimum test case for the fix this time? @akyrtzi

@akyrtzi
Copy link
Contributor

akyrtzi commented Jul 23, 2019

Adding a test case just for this fix is fine. Even though the existing test coverage is not great, we should at least add tests for any fixes we make.

@kitasuke
Copy link
Contributor Author

Sure, addressed your feedback.
I'll make a different PR to improve test coverage then.

@akyrtzi
Copy link
Contributor

akyrtzi commented Jul 24, 2019

@swift-ci test

@swiftlang swiftlang deleted a comment from swift-ci Jul 24, 2019
@akyrtzi akyrtzi merged commit 31aba31 into swiftlang:master Jul 24, 2019
@akyrtzi akyrtzi mentioned this pull request Jul 24, 2019
Merged
@kitasuke kitasuke deleted the SR-10241_bug_fix branch July 25, 2019 00:52
adevress pushed a commit to adevress/swift-syntax that referenced this pull request Jan 14, 2024
@allevato
Merge pull request swiftlang#135 from allevato/plain-old-unknowns
d4f58d3 
Flesh out unknown node handling.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@akyrtzi akyrtzi akyrtzi approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@kitasuke @akyrtzi