Skip to content
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 warning for invalid string sequences #694

Merged
merged 2 commits into from
Aug 8, 2019

Conversation

vladmos
Copy link
Member

@vladmos vladmos commented Aug 8, 2019

Fixes #688.

Copy link
Member

@pmbethe09 pmbethe09 left a comment

Choose a reason for hiding this comment

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

LGTM

var problems []int // positions of the problems

escaped := false
// This for-loop doesn't correclty check for a backlash at the end of the string literal, but
Copy link
Contributor

Choose a reason for hiding this comment

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

correctly

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

}

switch ch {
case '\n', '\\', 'n', 'r', 't', 'x', '\'', '"', '0', '1', '2', '3', '4', '5', '6', '7':
Copy link
Contributor

Choose a reason for hiding this comment

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

Where does the list come from?
Can you include a link?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.


var msg string
if len(problems) == 1 {
msg = fmt.Sprintf("Invalid quote sequences at position %d.", problems[0])
Copy link
Contributor

Choose a reason for hiding this comment

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

Users might be confused by the message. Include at least the character that's incorrectly escaped.

Suggested:
Invalid quote sequence '\w' at position 18.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

} else {
msg = fmt.Sprintf(
"Invalid quote sequences at positions %s.",
strings.Trim(strings.Join(strings.Fields(fmt.Sprint(problems)), ", "), "[]"),
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested:

Invalid quote sequences:
  '\w' at position 18
  '\z' at position 23

or:

Invalid quote sequences '\w', '\z' at positions 18, 23

Copy link
Member Author

Choose a reason for hiding this comment

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

Done (multiline warnings).

@vladmos vladmos merged commit 31b0229 into bazelbuild:master Aug 8, 2019
@vladmos vladmos deleted the string-escape branch August 8, 2019 15:00
cristiancreteanu pushed a commit to cristiancreteanu/buildtools that referenced this pull request Aug 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix string escapes
4 participants