Cole-Greer commented on code in PR #3673:
URL: https://github.com/apache/tinkerpop/pull/3673#discussion_r4126943198
##########
gremlin-js/gremlin-javascript/test/unit/translator/gremlin-translator-test.js:
##########
@@ -285,6 +285,14 @@ describe('GoTranslateVisitor', function () {
['g.inject(Duration(9000,0))', 'g.Inject(time.Duration(9000000000000))'],
// Binary literal
['g.inject(Binary("AQID"))', 'g.Inject(gremlingo.ByteBuffer{Data:
[]byte{1,2,3}})'],
+ // Character literals
+ ['g.inject("a"c)', "g.Inject(gremlingo.Char('a'))"],
+ ['g.inject("\\""c)', "g.Inject(gremlingo.Char('\"'))"],
Review Comment:
This is an invalid escape sequence, there shouldn't be any `\` here as the
double quote can exist on its own in a single quoted string.
```suggestion
['g.inject("\\""c)', "g.Inject(gremlingo.Char('"'))"],
```
##########
gremlin-js/gremlin-javascript/test/unit/translator/gremlin-translator-test.js:
##########
@@ -285,6 +285,14 @@ describe('GoTranslateVisitor', function () {
['g.inject(Duration(9000,0))', 'g.Inject(time.Duration(9000000000000))'],
// Binary literal
['g.inject(Binary("AQID"))', 'g.Inject(gremlingo.ByteBuffer{Data:
[]byte{1,2,3}})'],
+ // Character literals
+ ['g.inject("a"c)', "g.Inject(gremlingo.Char('a'))"],
+ ['g.inject("\\""c)', "g.Inject(gremlingo.Char('\"'))"],
+ ['g.inject("\\\\"c)', "g.Inject(gremlingo.Char('\\\\'))"],
Review Comment:
`'\\\\'` isn't a valid Rune in go (it translates to 2 literal backslash
characters), so `g.Inject(gremlingo.Char('\\\\'))` doesn't compile in Go.
I would also argue that `g.inject("\\\\"c)` shouldn't be considered a valid
character literal in GremlinLang for the same reason, although that's out of
scope for this PR. I would simply remove this case.
##########
gremlin-js/gremlin-javascript/test/unit/translator/gremlin-translator-test.js:
##########
@@ -285,6 +285,14 @@ describe('GoTranslateVisitor', function () {
['g.inject(Duration(9000,0))', 'g.Inject(time.Duration(9000000000000))'],
// Binary literal
['g.inject(Binary("AQID"))', 'g.Inject(gremlingo.ByteBuffer{Data:
[]byte{1,2,3}})'],
+ // Character literals
+ ['g.inject("a"c)', "g.Inject(gremlingo.Char('a'))"],
+ ['g.inject("\\""c)', "g.Inject(gremlingo.Char('\"'))"],
+ ['g.inject("\\\\"c)', "g.Inject(gremlingo.Char('\\\\'))"],
+ ["g.inject(\"'\"c)", "g.Inject(gremlingo.Char('\\''))"],
Review Comment:
Too many `\`, invalid escape sequence in Go.
```suggestion
["g.inject(\"'\"c)", "g.Inject(gremlingo.Char('\''))"],
```
##########
gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/language/translator/GremlinTranslatorTest.java:
##########
@@ -1437,7 +1437,7 @@ public static Collection<Object[]> data() {
null,
"g.inject(character0)",
"g.Inject<object>('a')",
- "Character literals are not supported in Go",
+ "g.Inject(gremlingo.Char('a'))",
Review Comment:
There's a bit of risk of growing the scope here, but I think we should
replicate the new translator tests from gremlin-translator-test.js here. The
java translators here are arguably more important than the JS ones, and it
appears the JS tests have revealed interesting edge cases (especially with
those octal escape sequences).
I wouldn't be surprised if these tests turn up issues in the other language
translators. If they are easy small fixes then perhaps we can tack them on
here, but if it reveals any non-trivial bugs I would say we should carve out a
new followup JIRA for them.
##########
gremlin-js/gremlin-javascript/test/unit/translator/gremlin-translator-test.js:
##########
@@ -285,6 +285,14 @@ describe('GoTranslateVisitor', function () {
['g.inject(Duration(9000,0))', 'g.Inject(time.Duration(9000000000000))'],
// Binary literal
['g.inject(Binary("AQID"))', 'g.Inject(gremlingo.ByteBuffer{Data:
[]byte{1,2,3}})'],
+ // Character literals
+ ['g.inject("a"c)', "g.Inject(gremlingo.Char('a'))"],
+ ['g.inject("\\""c)', "g.Inject(gremlingo.Char('\"'))"],
+ ['g.inject("\\\\"c)', "g.Inject(gremlingo.Char('\\\\'))"],
+ ["g.inject(\"'\"c)", "g.Inject(gremlingo.Char('\\''))"],
+ ["g.inject('\\''c)", "g.Inject(gremlingo.Char('\\''))"],
Review Comment:
```suggestion
["g.inject('\''c)", "g.Inject(gremlingo.Char('\''))"],
```
##########
gremlin-js/gremlin-javascript/test/unit/translator/gremlin-translator-test.js:
##########
@@ -285,6 +285,14 @@ describe('GoTranslateVisitor', function () {
['g.inject(Duration(9000,0))', 'g.Inject(time.Duration(9000000000000))'],
// Binary literal
['g.inject(Binary("AQID"))', 'g.Inject(gremlingo.ByteBuffer{Data:
[]byte{1,2,3}})'],
+ // Character literals
+ ['g.inject("a"c)', "g.Inject(gremlingo.Char('a'))"],
+ ['g.inject("\\""c)', "g.Inject(gremlingo.Char('\"'))"],
+ ['g.inject("\\\\"c)', "g.Inject(gremlingo.Char('\\\\'))"],
+ ["g.inject(\"'\"c)", "g.Inject(gremlingo.Char('\\''))"],
+ ["g.inject('\\''c)", "g.Inject(gremlingo.Char('\\''))"],
+ ['g.inject("\\7"c)', "g.Inject(gremlingo.Char('\\7'))"],
+ ['g.inject("\\07"c)', "g.Inject(gremlingo.Char('\\07'))"],
Review Comment:
Go doesn't support the same octal escape rules in its Runes as
GremlinLang/Java do in char literals. This probably needs a special case in the
translators to accommodate it, we should probably just translate it to octal
int literals in Go:
```suggestion
['g.inject("\7"c)', "g.Inject(gremlingo.Char(0o7))"],
['g.inject("\07"c)', "g.Inject(gremlingo.Char(0o7))"],
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
