-
Notifications
You must be signed in to change notification settings - Fork 472
Fix it to emit a single diagnostic when both open and close quote are missing #1703
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
Fix it to emit a single diagnostic when both open and close quote are missing #1703
Conversation
ahoppen
left a comment
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 way I would approach this is to add
public override func visit(_ node: StringLiteralExprSyntax) -> SyntaxVisitorContinueKind {to ParseDiagnosticsGenerator. If both node.openQuote and node.closeQuote are missing, you can generate a custom error message.
Ah, I see! You're referring to calling |
6e31834 to
8e56b23
Compare
8e56b23 to
e65a90e
Compare
ahoppen
left a comment
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. Looks very good. I only have a few minor comments to make it slightly more general
| fixIts: [#"insert '"'"#] | ||
| ), | ||
| message: #"expected 'abc' to be surrounded by '"'"#, | ||
| fixIts: [#"insert '""'"#] |
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 suspect that this will change to insert '"' and '"' after #1651 is merged.
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 letting me know! Once that PR is merged, I will rebase to apply the changes 🙂
e65a90e to
8847db8
Compare
ahoppen
left a comment
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. This looks great now. Can you rebase it now that #1651 is merged? Afterwards, I’ll trigger CI.
8847db8 to
2a455d0
Compare
|
@swift-ci please test |
|
@swift-ci please test windows |
Resolve #1691
When a string segment is located between missing quotes on both sides, looking ahead is interrupted, and each quote is diagnosed separately.
I have modified the logic to continue looking ahead even if a standalone string segment is encountered during the search.
Furthermore, I added a condition to the
parentContextClausein order to modify the diagnostic, but I am uncertain if it is the appropriate approach.If there is a better solution available, please let me know. Thank you.