You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tinkerpop.apache.org by "vkagamlyk (via GitHub)" <gi...@apache.org> on 2023/05/10 00:42:06 UTC
[GitHub] [tinkerpop] vkagamlyk opened a new pull request, #2059: Fixed bug in TextP.regex and TextP.notRegex serialization in Java
vkagamlyk opened a new pull request, #2059:
URL: https://github.com/apache/tinkerpop/pull/2059
Jira: https://issues.apache.org/jira/browse/TINKERPOP-2945
--
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
[GitHub] [tinkerpop] spmallette commented on a diff in pull request #2059: Fixed bug in TextP.regex and TextP.notRegex serialization in Java
Posted by "spmallette (via GitHub)" <gi...@apache.org>.
spmallette commented on code in PR #2059:
URL: https://github.com/apache/tinkerpop/pull/2059#discussion_r1190003886
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/translator/DotNetTranslator.java:
##########
@@ -463,17 +463,7 @@ protected Script produceScript(final String traversalSource, final Bytecode o) {
@Override
protected Script produceScript(final P<?> p) {
if (p instanceof TextP) {
- // special case the RegexPredicate since it isn't an enum. toString() for the final default will
- // typically cover implementations (generally worked for Text prior to 3.6.0)
- final BiPredicate<?, ?> tp = p.getBiPredicate();
- if (tp instanceof Text.RegexPredicate) {
- final String regexToken = ((Text.RegexPredicate) p.getBiPredicate()).isNegate() ? "NotRegex" : "Regex";
- script.append("TextP.").append(regexToken).append("(");
- } else if (tp instanceof Text) {
- script.append("TextP.").append(SymbolHelper.toCSharp(((Text) p.getBiPredicate()).name())).append("(");
- } else {
- script.append("TextP.").append(SymbolHelper.toCSharp(p.getBiPredicate().toString())).append("(");
- }
+ script.append("TextP.").append(SymbolHelper.toCSharp(p.getPredicateName())).append("(");
Review Comment:
nice
--
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
[GitHub] [tinkerpop] spmallette commented on a diff in pull request #2059: Fixed bug in TextP.regex and TextP.notRegex serialization in Java
Posted by "spmallette (via GitHub)" <gi...@apache.org>.
spmallette commented on code in PR #2059:
URL: https://github.com/apache/tinkerpop/pull/2059#discussion_r1190004814
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/io/graphson/TraversalSerializersV2d0.java:
##########
@@ -145,12 +145,7 @@ public PJacksonSerializer() {
public void serialize(final P p, final JsonGenerator jsonGenerator, final SerializerProvider serializerProvider)
throws IOException {
jsonGenerator.writeStartObject();
- jsonGenerator.writeStringField(GraphSONTokens.PREDICATE,
- p instanceof ConnectiveP ?
- p instanceof AndP ?
- GraphSONTokens.AND :
- GraphSONTokens.OR :
- p.getBiPredicate().toString());
+ jsonGenerator.writeStringField(GraphSONTokens.PREDICATE, p.getPredicateName());
Review Comment:
nice
--
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
[GitHub] [tinkerpop] vkagamlyk commented on a diff in pull request #2059: Fixed bug in TextP.regex and TextP.notRegex serialization in Java
Posted by "vkagamlyk (via GitHub)" <gi...@apache.org>.
vkagamlyk commented on code in PR #2059:
URL: https://github.com/apache/tinkerpop/pull/2059#discussion_r1190030702
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/P.java:
##########
@@ -57,6 +57,11 @@ public V getOriginalValue() {
return originalValue;
}
+ /*
+ * Get the name of the predicate
+ */
+ public String getPredicateName() { return biPredicate.toString(); }
Review Comment:
I thought to use a `name()` instead of `toString()`, by default they [return the same value](https://github.com/frohoff/jdk8u-dev-jdk/blob/master/src/share/classes/java/lang/Enum.java#L130), but `name()` is final and can't be overridden if necessary.
There is an option to add an interface over `BiPredicate` with additional `getPredicateName()` and change `P` to accept only instances of this type, like `public P(final PPredicate<V, V> pPredicate, final V value)`. But I thought it was too big change for `3.6` and left it as is
--
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
[GitHub] [tinkerpop] vkagamlyk merged pull request #2059: [TINKERPOP-2945] Fixed bug in TextP.regex and TextP.notRegex serialization in Java
Posted by "vkagamlyk (via GitHub)" <gi...@apache.org>.
vkagamlyk merged PR #2059:
URL: https://github.com/apache/tinkerpop/pull/2059
--
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
[GitHub] [tinkerpop] spmallette commented on a diff in pull request #2059: Fixed bug in TextP.regex and TextP.notRegex serialization in Java
Posted by "spmallette (via GitHub)" <gi...@apache.org>.
spmallette commented on code in PR #2059:
URL: https://github.com/apache/tinkerpop/pull/2059#discussion_r1190045745
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/P.java:
##########
@@ -57,6 +57,11 @@ public V getOriginalValue() {
return originalValue;
}
+ /*
+ * Get the name of the predicate
+ */
+ public String getPredicateName() { return biPredicate.toString(); }
Review Comment:
> I thought to use a name() instead of toString(), by default they return the same value, but name() is final and can't be overridden if necessary.
hmm - interesting. i wonder if overriding would ever make sense because doing so would just break the serializer, right? your call, if you want to change it or leave it as-is.
> There is an option to add an interface over BiPredicate with additional getPredicateName() and change P to accept only instances of this type, like public P(final PPredicate<V, V> pPredicate, final V value). But I thought it was too big change for 3.6 and left it as is
that's fair. maybe just create a JIRA to implement a bigger change later (maybe 3.7.x). maybe `P` should never take a naked `BiPredicate`? anyway, i suppose that's for future discussion.
--
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
[GitHub] [tinkerpop] spmallette commented on pull request #2059: Fixed bug in TextP.regex and TextP.notRegex serialization in Java
Posted by "spmallette (via GitHub)" <gi...@apache.org>.
spmallette commented on PR #2059:
URL: https://github.com/apache/tinkerpop/pull/2059#issuecomment-1542570467
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
[GitHub] [tinkerpop] vkagamlyk commented on a diff in pull request #2059: Fixed bug in TextP.regex and TextP.notRegex serialization in Java
Posted by "vkagamlyk (via GitHub)" <gi...@apache.org>.
vkagamlyk commented on code in PR #2059:
URL: https://github.com/apache/tinkerpop/pull/2059#discussion_r1189990438
##########
CHANGELOG.asciidoc:
##########
@@ -25,7 +25,9 @@ image::https://raw.githubusercontent.com/apache/tinkerpop/master/docs/static/ima
This release also includes changes from <<release-3-5-7, 3.5.7>>.
+==== Bugs
Review Comment:
removed at https://github.com/apache/tinkerpop/pull/2059/commits/6c5bac160f237316eabbd697522b203b05570768
--
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
[GitHub] [tinkerpop] xiazcy commented on pull request #2059: [TINKERPOP-2945] Fixed bug in TextP.regex and TextP.notRegex serialization in Java
Posted by "xiazcy (via GitHub)" <gi...@apache.org>.
xiazcy commented on PR #2059:
URL: https://github.com/apache/tinkerpop/pull/2059#issuecomment-1544900509
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
[GitHub] [tinkerpop] kenhuuu commented on pull request #2059: [TINKERPOP-2945] Fixed bug in TextP.regex and TextP.notRegex serialization in Java
Posted by "kenhuuu (via GitHub)" <gi...@apache.org>.
kenhuuu commented on PR #2059:
URL: https://github.com/apache/tinkerpop/pull/2059#issuecomment-1544685727
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
[GitHub] [tinkerpop] codecov-commenter commented on pull request #2059: Fixed bug in TextP.regex and TextP.notRegex serialization in Java
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #2059:
URL: https://github.com/apache/tinkerpop/pull/2059#issuecomment-1541098472
## [Codecov](https://app.codecov.io/gh/apache/tinkerpop/pull/2059?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#2059](https://app.codecov.io/gh/apache/tinkerpop/pull/2059?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (ee9394c) into [3.6-dev](https://app.codecov.io/gh/apache/tinkerpop/commit/4b51fd24075a3ef2ba044cc01670378bce9feafa?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (4b51fd2) will **decrease** coverage by `4.95%`.
> The diff coverage is `n/a`.
```diff
@@ Coverage Diff @@
## 3.6-dev #2059 +/- ##
=============================================
- Coverage 69.54% 64.59% -4.95%
=============================================
Files 878 25 -853
Lines 42283 3802 -38481
Branches 5655 0 -5655
=============================================
- Hits 29406 2456 -26950
+ Misses 10888 1179 -9709
+ Partials 1989 167 -1822
```
[see 853 files with indirect coverage changes](https://app.codecov.io/gh/apache/tinkerpop/pull/2059/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
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
[GitHub] [tinkerpop] spmallette commented on a diff in pull request #2059: Fixed bug in TextP.regex and TextP.notRegex serialization in Java
Posted by "spmallette (via GitHub)" <gi...@apache.org>.
spmallette commented on code in PR #2059:
URL: https://github.com/apache/tinkerpop/pull/2059#discussion_r1189985719
##########
CHANGELOG.asciidoc:
##########
@@ -25,7 +25,9 @@ image::https://raw.githubusercontent.com/apache/tinkerpop/master/docs/static/ima
This release also includes changes from <<release-3-5-7, 3.5.7>>.
+==== Bugs
Review Comment:
please remove title. that's add on release
--
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
[GitHub] [tinkerpop] vkagamlyk commented on pull request #2059: [TINKERPOP-2945] Fixed bug in TextP.regex and TextP.notRegex serialization in Java
Posted by "vkagamlyk (via GitHub)" <gi...@apache.org>.
vkagamlyk commented on PR #2059:
URL: https://github.com/apache/tinkerpop/pull/2059#issuecomment-1544900541
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
[GitHub] [tinkerpop] spmallette commented on a diff in pull request #2059: Fixed bug in TextP.regex and TextP.notRegex serialization in Java
Posted by "spmallette (via GitHub)" <gi...@apache.org>.
spmallette commented on code in PR #2059:
URL: https://github.com/apache/tinkerpop/pull/2059#discussion_r1190003122
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/P.java:
##########
@@ -57,6 +57,11 @@ public V getOriginalValue() {
return originalValue;
}
+ /*
+ * Get the name of the predicate
+ */
+ public String getPredicateName() { return biPredicate.toString(); }
Review Comment:
just off the top of my head, `biPredicate` is usually an `Enum`. could we avoid future problems potentially if we tested for `Enum` and then called `name()` and restore to `toString()` for all other cases. I guess another question is whether a random `BiPredicate` even makes sense to have a predicate name? Like, is there a predicate name for a lambda and what use would that be in serialization? it would just cause an exception in serialization. is that the best way for that error to raise?
--
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