-
Notifications
You must be signed in to change notification settings - Fork 0
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
Escape character support for string literals #159
Conversation
Signed-off-by: MitchellGale-BitQuill <[email protected]>
Signed-off-by: MitchellGale-BitQuill <[email protected]>
Codecov Report
@@ Coverage Diff @@
## Integ-escapeCharacters #159 +/- ##
=========================================================
Coverage 98.31% 98.31%
Complexity 3524 3524
=========================================================
Files 342 342
Lines 8711 8711
Branches 555 555
=========================================================
Hits 8564 8564
Misses 142 142
Partials 5 5
Flags with carried forward coverage won't be shown. Click here to find out more. 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Signed-off-by: MitchellGale-BitQuill <[email protected]>
MySQL doesn't support with back ticks for doing fancy escape stuff, but the old implementation did. What should we go with? |
Also removed unused StringUtils function Signed-off-by: MitchellGale-BitQuill <[email protected]>
Signed-off-by: MitchellGale-BitQuill <[email protected]>
Signed-off-by: MitchellGale-BitQuill <[email protected]>
if (currentChar == enclosingQuote | ||
&& nextChar == currentChar) { | ||
|
||
if ((currentChar == '\\' |
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.
logic here is very hard to follow (containing both && and || and lots of brackets. I'd suggest simplifying it.
May have one if statement for currentChar == '\\'
and another for currentChar == enclosingQuote
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.
Definitely need to put a set of brackets around
currentChar == nextChar
&& currentChar == enclosingQuote
|
||
|
||
public class StringLiteralIT extends SQLIntegTestCase { | ||
// @Test |
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.
TODO
Signed-off-by: MitchellGale-BitQuill <[email protected]>
Signed-off-by: MitchellGale-BitQuill <[email protected]>
Signed-off-by: MitchellGale-BitQuill <[email protected]>
Signed-off-by: MitchellGale-BitQuill <[email protected]>
Signed-off-by: MitchellGale-BitQuill <[email protected]>
Signed-off-by: MitchellGale-BitQuill <[email protected]>
if (currentChar == enclosingQuote | ||
&& nextChar == currentChar) { | ||
|
||
if ((currentChar == '\\' |
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.
Definitely need to put a set of brackets around
currentChar == nextChar
&& currentChar == enclosingQuote
Does this make that test in TDVT pass? Might be good to check as that is the issue being solved by this PR. |
Signed-off-by: MitchellGale-BitQuill <[email protected]>
Signed-off-by: MitchellGale-BitQuill <[email protected]>
Signed-off-by: MitchellGale-BitQuill <[email protected]>
Signed-off-by: MitchellGale-BitQuill <[email protected]>
Description
Adds support for escape characters
\
to be used to escape characters in string literals. Keeps support for double quote to act as an escape if the surrounding quote is matching type (single quote, or double quote).Issues Resolved
opensearch-project#297
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.