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

[TINKERPOP-2966] Change PythonTranslator to generate underscore based step naming #3039

Open
wants to merge 1 commit into
base: 3.7-dev
Choose a base branch
from

Conversation

flora-jin
Copy link
Contributor

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

@codecov-commenter
Copy link

codecov-commenter commented Feb 13, 2025

Codecov Report

Attention: Patch coverage is 84.61538% with 2 lines in your changes missing coverage. Please review.

Project coverage is 76.36%. Comparing base (9b46b67) to head (2ca9647).
Report is 313 commits behind head on 3.7-dev.

Files with missing lines Patch % Lines
...process/traversal/translator/PythonTranslator.java 84.61% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@              Coverage Diff              @@
##             3.7-dev    #3039      +/-   ##
=============================================
+ Coverage      76.14%   76.36%   +0.22%     
- Complexity     13152    13232      +80     
=============================================
  Files           1084     1061      -23     
  Lines          65160    61567    -3593     
  Branches        7285     7346      +61     
=============================================
- Hits           49616    47016    -2600     
+ Misses         12839    12033     -806     
+ Partials        2705     2518     -187     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

}

public static String convertCamelCaseToSnakeCase(final String camelCase) {
if (camelCase == null || camelCase.isEmpty())
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: you can use StringUtils.isBlank(camelCase) which will also check for other types of whitespace.

return camelCase;

final StringBuilder sb = new StringBuilder();
for (int i = 0; i < camelCase.length(); i++) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of looping through each character can you use replaceAll?

return camelCase.replaceAll("([A-Z])", "_$1").toLowerCase();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@@ -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");
Copy link
Contributor

Choose a reason for hiding this comment

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

Ideally there would be unit test coverage in the PythonTranslatorTest to check each of these removals didn't break the translator functionality.

@@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this should have been changed as it's groovy not python.

@@ -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')))],
Copy link
Contributor

Choose a reason for hiding this comment

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

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

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));
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants