You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tinkerpop.apache.org by "flora-jin (via GitHub)" <gi...@apache.org> on 2025/02/13 22:46:02 UTC
[PR] [TINKERPOP-2966] Change PythonTranslator to generate underscore based step naming [tinkerpop]
flora-jin opened a new pull request, #3039:
URL: https://github.com/apache/tinkerpop/pull/3039
https://issues.apache.org/jira/browse/TINKERPOP-2966
The more idiomatic underscore based step naming was introduced a long time ago but the PythonTranslator} still produces the old camelcase style.
- Updated PythonTranslator to generate snake case step naming instead of camel case. Referred to 4.0 PythonTranslator changes.
- Removed some static naming in hash map
- Updated tests that required camel case step naming
- Changed ChangeLog
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [TINKERPOP-2966] Change PythonTranslator to generate underscore based step naming [tinkerpop]
Posted by "flora-jin (via GitHub)" <gi...@apache.org>.
flora-jin commented on code in PR #3039:
URL: https://github.com/apache/tinkerpop/pull/3039#discussion_r1961951019
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/translator/PythonTranslator.java:
##########
@@ -479,9 +467,28 @@ private SymbolHelper() {
}
public static String toPython(final String symbol) {
- // at some point we will want a camel to snake case converter here. for now the only step that needs
- // this conversion is mergeE/V related as the rest still continue use in their deprecated forms.
- return TO_PYTHON_MAP.getOrDefault(symbol, symbol);
+ return TO_PYTHON_MAP.getOrDefault(symbol, convertCamelCaseToSnakeCase(symbol));
+ }
+
+ public static String convertCamelCaseToSnakeCase(final String camelCase) {
+ if (camelCase == null || camelCase.isEmpty())
+ return camelCase;
+
+ // skip if this is a class/enum indicated by the first letter being upper case
+ if (Character.isUpperCase(camelCase.charAt(0)))
+ return camelCase;
+
+ final StringBuilder sb = new StringBuilder();
+ for (int i = 0; i < camelCase.length(); i++) {
Review Comment:
Yes I was thinking a similar thing but this is following the changes that were completed on 4.0. I wasn't sure if it would be okay for 3.7 to have a different implementation than 4.0. If alright though, I also think it would be easier to use the `replaceAll`
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [TINKERPOP-2966] Change PythonTranslator to generate underscore based step naming [tinkerpop]
Posted by "andreachild (via GitHub)" <gi...@apache.org>.
andreachild commented on code in PR #3039:
URL: https://github.com/apache/tinkerpop/pull/3039#discussion_r1960663089
##########
gremlin-groovy/src/test/java/org/apache/tinkerpop/gremlin/groovy/jsr223/GremlinGroovyScriptEngineTest.java:
##########
@@ -515,7 +515,7 @@ public void shouldProduceBindingsForVars() throws Exception {
assertThat(bytecodeBindings.containsKey("two"), is(true));
assertThat(bytecodeBindings.containsKey("three"), is(true));
- assertEquals("g.V(v1Id).has('person','age',29).has('person','active',x).in_('knows').merge_e(m1).merge_v(m2).option(Merge.on_create,m3).merge_v(__.identity()).choose(__.out().count()).option(two,__.name).option(three,__.age).filter_(__.outE().count().is_(y)).map(l).order().by('name',o)", gremlinAsPython);
+ assertEquals("g.V(v1Id).has('person','age',29).has('person','active',x).in_('knows').merge_e(m1).merge_v(m2).option(Merge.on_create,m3).merge_v(__.identity()).choose(__.out().count()).option(two,__.name).option(three,__.age).filter_(__.out_e().count().is_(y)).map(l).order().by('name',o)", gremlinAsPython);
Review Comment:
I don't think this should have been changed as it's groovy not python.
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [TINKERPOP-2966] Change PythonTranslator to generate underscore based step naming [tinkerpop]
Posted by "andreachild (via GitHub)" <gi...@apache.org>.
andreachild commented on code in PR #3039:
URL: https://github.com/apache/tinkerpop/pull/3039#discussion_r1960666187
##########
gremlin-python/src/main/python/radish/gremlin.py:
##########
@@ -37,48 +37,48 @@
'g_V_branchXlabel_eq_person__a_bX_optionXa__ageX_optionXb__langX_optionXb__nameX': [(lambda g, l1=None:g.V().branch(l1).option('a',__.age).option('b',__.lang).option('b',__.name))],
'g_V_branchXlabel_isXpersonX_countX_optionX1__ageX_optionX0__langX_optionX0__nameX': [(lambda g, xx1=None,xx2=None:g.V().branch(__.label().is_('person').count()).option(xx1,__.age).option(xx2,__.lang).option(xx2,__.name))],
'g_V_branchXlabel_isXpersonX_countX_optionX1__ageX_optionX0__langX_optionX0__nameX_optionXany__labelX': [(lambda g, xx1=None,xx2=None:g.V().branch(__.label().is_('person').count()).option(xx1,__.age).option(xx2,__.lang).option(xx2,__.name).option(Pick.any_,__.label()))],
- 'g_V_branchXageX_optionXltX30X__youngX_optionXgtX30X__oldX_optionXnone__on_the_edgeX': [(lambda g:g.V().hasLabel('person').branch(__.age).option(P.lt(30),__.constant('young')).option(P.gt(30),__.constant('old')).option(Pick.none,__.constant('on the edge')))],
Review Comment:
Did you modify this file manually? I wouldn't think any of your changes would have caused this file to be modified (it is auto-generated).
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [TINKERPOP-2966] Change PythonTranslator to generate underscore based step naming [tinkerpop]
Posted by "flora-jin (via GitHub)" <gi...@apache.org>.
flora-jin commented on code in PR #3039:
URL: https://github.com/apache/tinkerpop/pull/3039#discussion_r1961953980
##########
gremlin-python/src/main/python/radish/gremlin.py:
##########
@@ -37,48 +37,48 @@
'g_V_branchXlabel_eq_person__a_bX_optionXa__ageX_optionXb__langX_optionXb__nameX': [(lambda g, l1=None:g.V().branch(l1).option('a',__.age).option('b',__.lang).option('b',__.name))],
'g_V_branchXlabel_isXpersonX_countX_optionX1__ageX_optionX0__langX_optionX0__nameX': [(lambda g, xx1=None,xx2=None:g.V().branch(__.label().is_('person').count()).option(xx1,__.age).option(xx2,__.lang).option(xx2,__.name))],
'g_V_branchXlabel_isXpersonX_countX_optionX1__ageX_optionX0__langX_optionX0__nameX_optionXany__labelX': [(lambda g, xx1=None,xx2=None:g.V().branch(__.label().is_('person').count()).option(xx1,__.age).option(xx2,__.lang).option(xx2,__.name).option(Pick.any_,__.label()))],
- 'g_V_branchXageX_optionXltX30X__youngX_optionXgtX30X__oldX_optionXnone__on_the_edgeX': [(lambda g:g.V().hasLabel('person').branch(__.age).option(P.lt(30),__.constant('young')).option(P.gt(30),__.constant('old')).option(Pick.none,__.constant('on the edge')))],
Review Comment:
Nope did not edit manually. It is generated so when I changed the PythonTranslator file, and ran maven it auto-generated with the replaced snake style name spacing.
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [TINKERPOP-2966] Change PythonTranslator to generate underscore based step naming [tinkerpop]
Posted by "andreachild (via GitHub)" <gi...@apache.org>.
andreachild commented on code in PR #3039:
URL: https://github.com/apache/tinkerpop/pull/3039#discussion_r1960638400
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/translator/PythonTranslator.java:
##########
@@ -479,9 +467,28 @@ private SymbolHelper() {
}
public static String toPython(final String symbol) {
- // at some point we will want a camel to snake case converter here. for now the only step that needs
- // this conversion is mergeE/V related as the rest still continue use in their deprecated forms.
- return TO_PYTHON_MAP.getOrDefault(symbol, symbol);
+ return TO_PYTHON_MAP.getOrDefault(symbol, convertCamelCaseToSnakeCase(symbol));
+ }
+
+ public static String convertCamelCaseToSnakeCase(final String camelCase) {
+ if (camelCase == null || camelCase.isEmpty())
Review Comment:
Nit: you can use `StringUtils.isBlank(camelCase)` which will also check for other types of whitespace.
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [TINKERPOP-2966] Change PythonTranslator to generate underscore based step naming [tinkerpop]
Posted by "Cole-Greer (via GitHub)" <gi...@apache.org>.
Cole-Greer merged PR #3039:
URL: https://github.com/apache/tinkerpop/pull/3039
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [TINKERPOP-2966] Change PythonTranslator to generate underscore based step naming [tinkerpop]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #3039:
URL: https://github.com/apache/tinkerpop/pull/3039#issuecomment-2657895436
## [Codecov](https://app.codecov.io/gh/apache/tinkerpop/pull/3039?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
Attention: Patch coverage is `84.61538%` with `2 lines` in your changes missing coverage. Please review.
> Project coverage is 76.35%. Comparing base [(`9b46b67`)](https://app.codecov.io/gh/apache/tinkerpop/commit/9b46b6777d2fa250e41daacf2fa4554605aff53a?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`8babd3e`)](https://app.codecov.io/gh/apache/tinkerpop/commit/8babd3e7f822884edfa4cd5daa17bf35bb85ebc8?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
> Report is 312 commits behind head on 3.7-dev.
| [Files with missing lines](https://app.codecov.io/gh/apache/tinkerpop/pull/3039?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...process/traversal/translator/PythonTranslator.java](https://app.codecov.io/gh/apache/tinkerpop/pull/3039?src=pr&el=tree&filepath=gremlin-core%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Ftinkerpop%2Fgremlin%2Fprocess%2Ftraversal%2Ftranslator%2FPythonTranslator.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Z3JlbWxpbi1jb3JlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS90aW5rZXJwb3AvZ3JlbWxpbi9wcm9jZXNzL3RyYXZlcnNhbC90cmFuc2xhdG9yL1B5dGhvblRyYW5zbGF0b3IuamF2YQ==) | 84.61% | [1 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/tinkerpop/pull/3039?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## 3.7-dev #3039 +/- ##
=============================================
+ Coverage 76.14% 76.35% +0.21%
- Complexity 13152 13228 +76
=============================================
Files 1084 1061 -23
Lines 65160 61567 -3593
Branches 7285 7346 +61
=============================================
- Hits 49616 47012 -2604
+ Misses 12839 12033 -806
+ Partials 2705 2522 -183
```
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/tinkerpop/pull/3039?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [TINKERPOP-2966] Change PythonTranslator to generate underscore based step naming [tinkerpop]
Posted by "Cole-Greer (via GitHub)" <gi...@apache.org>.
Cole-Greer commented on PR #3039:
URL: https://github.com/apache/tinkerpop/pull/3039#issuecomment-2698590327
VOTE +1
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [TINKERPOP-2966] Change PythonTranslator to generate underscore based step naming [tinkerpop]
Posted by "andreachild (via GitHub)" <gi...@apache.org>.
andreachild commented on PR #3039:
URL: https://github.com/apache/tinkerpop/pull/3039#issuecomment-2688615980
VOTE +1
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [TINKERPOP-2966] Change PythonTranslator to generate underscore based step naming [tinkerpop]
Posted by "andreachild (via GitHub)" <gi...@apache.org>.
andreachild commented on code in PR #3039:
URL: https://github.com/apache/tinkerpop/pull/3039#discussion_r1960660296
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/translator/PythonTranslator.java:
##########
@@ -443,10 +443,6 @@ static final class SymbolHelper {
TO_PYTHON_MAP.put("and", "and_");
TO_PYTHON_MAP.put("any", "any_");
TO_PYTHON_MAP.put("as", "as_");
- TO_PYTHON_MAP.put("asString", "as_string");
Review Comment:
Ideally there would be unit test coverage in the `PythonTranslatorTest` to check each of these removals didn't break the translator functionality.
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [TINKERPOP-2966] Change PythonTranslator to generate underscore based step naming [tinkerpop]
Posted by "kenhuuu (via GitHub)" <gi...@apache.org>.
kenhuuu commented on PR #3039:
URL: https://github.com/apache/tinkerpop/pull/3039#issuecomment-2691651905
VOTE +1
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [TINKERPOP-2966] Change PythonTranslator to generate underscore based step naming [tinkerpop]
Posted by "flora-jin (via GitHub)" <gi...@apache.org>.
flora-jin commented on code in PR #3039:
URL: https://github.com/apache/tinkerpop/pull/3039#discussion_r1965997713
##########
gremlin-groovy/src/test/java/org/apache/tinkerpop/gremlin/groovy/jsr223/GremlinGroovyScriptEngineTest.java:
##########
@@ -515,7 +515,7 @@ public void shouldProduceBindingsForVars() throws Exception {
assertThat(bytecodeBindings.containsKey("two"), is(true));
assertThat(bytecodeBindings.containsKey("three"), is(true));
- assertEquals("g.V(v1Id).has('person','age',29).has('person','active',x).in_('knows').merge_e(m1).merge_v(m2).option(Merge.on_create,m3).merge_v(__.identity()).choose(__.out().count()).option(two,__.name).option(three,__.age).filter_(__.outE().count().is_(y)).map(l).order().by('name',o)", gremlinAsPython);
+ assertEquals("g.V(v1Id).has('person','age',29).has('person','active',x).in_('knows').merge_e(m1).merge_v(m2).option(Merge.on_create,m3).merge_v(__.identity()).choose(__.out().count()).option(two,__.name).option(three,__.age).filter_(__.out_e().count().is_(y)).map(l).order().by('name',o)", gremlinAsPython);
Review Comment:
I changed it as when I ran all tests this one broke with expected as the snake style
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [TINKERPOP-2966] Change PythonTranslator to generate underscore based step naming [tinkerpop]
Posted by "flora-jin (via GitHub)" <gi...@apache.org>.
flora-jin commented on code in PR #3039:
URL: https://github.com/apache/tinkerpop/pull/3039#discussion_r1970390371
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/translator/PythonTranslator.java:
##########
@@ -443,10 +443,6 @@ static final class SymbolHelper {
TO_PYTHON_MAP.put("and", "and_");
TO_PYTHON_MAP.put("any", "any_");
TO_PYTHON_MAP.put("as", "as_");
- TO_PYTHON_MAP.put("asString", "as_string");
Review Comment:
Checked that all of the removals are present in gremlin.py
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [TINKERPOP-2966] Change PythonTranslator to generate underscore based step naming [tinkerpop]
Posted by "andreachild (via GitHub)" <gi...@apache.org>.
andreachild commented on code in PR #3039:
URL: https://github.com/apache/tinkerpop/pull/3039#discussion_r1960656791
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/translator/PythonTranslator.java:
##########
@@ -479,9 +467,28 @@ private SymbolHelper() {
}
public static String toPython(final String symbol) {
- // at some point we will want a camel to snake case converter here. for now the only step that needs
- // this conversion is mergeE/V related as the rest still continue use in their deprecated forms.
- return TO_PYTHON_MAP.getOrDefault(symbol, symbol);
+ return TO_PYTHON_MAP.getOrDefault(symbol, convertCamelCaseToSnakeCase(symbol));
+ }
+
+ public static String convertCamelCaseToSnakeCase(final String camelCase) {
+ if (camelCase == null || camelCase.isEmpty())
+ return camelCase;
+
+ // skip if this is a class/enum indicated by the first letter being upper case
+ if (Character.isUpperCase(camelCase.charAt(0)))
+ return camelCase;
+
+ final StringBuilder sb = new StringBuilder();
+ for (int i = 0; i < camelCase.length(); i++) {
Review Comment:
Instead of looping through each character can you use `replaceAll`?
```
return camelCase.replaceAll("([A-Z])", "_$1").toLowerCase();
```
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [TINKERPOP-2966] Change PythonTranslator to generate underscore based step naming [tinkerpop]
Posted by "andreachild (via GitHub)" <gi...@apache.org>.
andreachild commented on code in PR #3039:
URL: https://github.com/apache/tinkerpop/pull/3039#discussion_r1962013380
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/translator/PythonTranslator.java:
##########
@@ -455,20 +451,12 @@ static final class SymbolHelper {
TO_PYTHON_MAP.put("is", "is_");
TO_PYTHON_MAP.put("list", "list_");
TO_PYTHON_MAP.put("max", "max_");
- TO_PYTHON_MAP.put("mergeE", "merge_e");
- TO_PYTHON_MAP.put("mergeV", "merge_v");
- TO_PYTHON_MAP.put("inV", "in_v");
- TO_PYTHON_MAP.put("outV", "out_v");
- TO_PYTHON_MAP.put("onCreate", "on_create");
- TO_PYTHON_MAP.put("onMatch", "on_match");
TO_PYTHON_MAP.put("min", "min_");
TO_PYTHON_MAP.put("or", "or_");
TO_PYTHON_MAP.put("not", "not_");
TO_PYTHON_MAP.put("range", "range_");
TO_PYTHON_MAP.put("set", "set_");
TO_PYTHON_MAP.put("sum", "sum_");
- TO_PYTHON_MAP.put("toLower", "to_lower");
- TO_PYTHON_MAP.put("toUpper", "to_upper");
TO_PYTHON_MAP.put("with", "with_");
//
TO_PYTHON_MAP.forEach((k, v) -> FROM_PYTHON_MAP.put(v, k));
Review Comment:
The removals of entries from the `TO_PYTHON_MAP` also affect the `FROM_PYTHON_MAP` which affects the `toJava` method. It doesn't look like the `toJava` method is actually used so both the `toJava` method and `FROM_PYTHON_MAP` should be removed.
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org