-
Notifications
You must be signed in to change notification settings - Fork 819
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
base: 3.7-dev
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
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. |
… accordingly, and changed ChangeLog
dfd3f15
to
2ca9647
Compare
} | ||
|
||
public static String convertCamelCaseToSnakeCase(final String camelCase) { | ||
if (camelCase == null || camelCase.isEmpty()) |
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.
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++) { |
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.
Instead of looping through each character can you use replaceAll
?
return camelCase.replaceAll("([A-Z])", "_$1").toLowerCase();
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.
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"); |
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.
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); |
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.
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')))], |
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.
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).
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.
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)); |
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.
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.
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.