kenhuuu commented on PR #3336: URL: https://github.com/apache/tinkerpop/pull/3336#issuecomment-4115104500
I have limitted knowledge in the translators but I tried to follow the instructions in the description about how to review this PR. Generating the test data from the feature tests seems reasonable to me but there are definitely some gaps there. Until recently, many of the strategies didn't even have tests. There are no tests tx()-related parts of the grammar, some terminal steps are missing, parts of the language that I think were borrowed from Groovy but don't really exist in other languages like `1..10` (I guess these can't get translated?). But based on the testing that is done for the translators implemented in Java, the coverage is probably good enough. However, it seems like the GremlinTranslatorTest isn't really a one for one with the newly added gremlin-translator-test. Why is that the case? I don't think the catch in `normalizeAndTranslate()` is a big deal, I guess we could try to make that an option, but I think it makes sense to just attempt to run it through the translator like it is now. You should probably clarify what canonical casing actually is for enums since the examples there suggest PascalCase but we have enums like `DT`. We might need something there about using "Infinity" and "NaN" instead of language specific versions. Yes, this probably does have injection problems, but the end result of translation should still be valid Gremlin. I think it'll have to be on the user to have certain permissions setup to allow what kinds of Gremlin can run on their Graph. That last point about offline mode is well beyond my JavaScript/TypeScript knowledge so I just took a brief look over it. -- 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]
