You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tinkerpop.apache.org by dkuppitz <gi...@git.apache.org> on 2016/01/22 18:15:44 UTC
[GitHub] incubator-tinkerpop pull request: TINKERPOP-818 Consider a P.type(...
GitHub user dkuppitz opened a pull request:
https://github.com/apache/incubator-tinkerpop/pull/206
TINKERPOP-818 Consider a P.type()
I'm not happy with the naming of the opposite of `P.type()`: `P.notType()`
Any suggestions are welcome and the refactoring should be pretty easy. But so far ...
* `mvn clean install`: passed
* integration tests: passed
VOTE: +1
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/apache/incubator-tinkerpop TINKERPOP-818
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-tinkerpop/pull/206.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #206
----
commit 11845b7a0408f910fed96a261369c315b3a39773
Author: Daniel Kuppitz <da...@hotmail.com>
Date: 2016-01-22T17:09:47Z
Added `P.type()`, `P.notType()`, `Compare.type()` and `Compare.notType()`.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-tinkerpop pull request: TINKERPOP-818 Consider a P.type(...
Posted by dkuppitz <gi...@git.apache.org>.
Github user dkuppitz commented on the pull request:
https://github.com/apache/incubator-tinkerpop/pull/206#issuecomment-173990024
> there should NOT be `notType()` that is `not(type(Class))`
I fully agree, BUT I had to [implement negate()](https://github.com/apache/incubator-tinkerpop/blob/11845b7a0408f910fed96a261369c315b3a39773/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/Compare.java#L190-L193).
> Use cases?
We've seen use cases in the past I just can't provide one off my head.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-tinkerpop pull request: TINKERPOP-818 Consider a P.type(...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on the pull request:
https://github.com/apache/incubator-tinkerpop/pull/206#issuecomment-221401320
cool - please close up the JIRA issue too if the intention is to never do this at all.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-tinkerpop pull request: TINKERPOP-818 Consider a P.type(...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on the pull request:
https://github.com/apache/incubator-tinkerpop/pull/206#issuecomment-221395541
@dkuppitz This PR kinda died - is it worth keeping it open?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-tinkerpop pull request: TINKERPOP-818 Consider a P.type(...
Posted by okram <gi...@git.apache.org>.
Github user okram commented on the pull request:
https://github.com/apache/incubator-tinkerpop/pull/206#issuecomment-174004228
I don't see how `type` is a comparable? Its not sortable.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-tinkerpop pull request: TINKERPOP-818 Consider a P.type(...
Posted by okram <gi...@git.apache.org>.
Github user okram commented on the pull request:
https://github.com/apache/incubator-tinkerpop/pull/206#issuecomment-173984732
I don't think types should be comparable. Its equals or not equals. Next, there should NOT be `notType()` that is `not(type(Class))`. Also, are there any good reasons for this? Use cases?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-tinkerpop pull request: TINKERPOP-818 Consider a P.type(...
Posted by okram <gi...@git.apache.org>.
Github user okram commented on the pull request:
https://github.com/apache/incubator-tinkerpop/pull/206#issuecomment-174571662
Couple of notes.
* Why is `Type` an enum?
* `Type` will need to implement serializable.
* Is `Type` the best name? Perhaps `TypePredicate`? We should really hide `Type` as the only way to get to it is via `P`.
* I would not do the `String` model just yet. Stick with just a `Class` parameter until we think through fully what this means for other languages.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-tinkerpop pull request: TINKERPOP-818 Consider a P.type(...
Posted by dkuppitz <gi...@git.apache.org>.
Github user dkuppitz commented on the pull request:
https://github.com/apache/incubator-tinkerpop/pull/206#issuecomment-221398753
Probably not. As @okram said "limited use cases" and then there's an open question on how to handle those things properly in different languages. Since nobody asked for it within the last 5-6 months nor did I need it, we should in fact "wait until there is a stress for it". I will close the PR and then delete the branch tomorrow if there are no objections.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-tinkerpop pull request: TINKERPOP-818 Consider a P.type(...
Posted by dkuppitz <gi...@git.apache.org>.
Github user dkuppitz closed the pull request at:
https://github.com/apache/incubator-tinkerpop/pull/206
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-tinkerpop pull request: TINKERPOP-818 Consider a P.type(...
Posted by dkuppitz <gi...@git.apache.org>.
Github user dkuppitz commented on the pull request:
https://github.com/apache/incubator-tinkerpop/pull/206#issuecomment-174000234
Do you suggest to implement the logic directly in `P`? So far the pattern has always been: implement the `BiPredicate` (`Compare` or `Contains`) and use it in `P`. For other language we could allow the type to be a `String`: `g.V()...is(type("java.lang.String"))`
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-tinkerpop pull request: TINKERPOP-818 Consider a P.type(...
Posted by okram <gi...@git.apache.org>.
Github user okram commented on the pull request:
https://github.com/apache/incubator-tinkerpop/pull/206#issuecomment-173991134
You implemented `Compare` not `P`. If we do `type`, it needs to be a `P`-predicate. (and still assuming we want this...). I just haven't seen anyone (nor myself) ever need this functionality. And its bad to add functionality with limited use cases. Perhaps we wait until there is a stress for it AND we know all the ways in which it will be used. For example -- by doing this, we are assuming the Java-type system. This may be bad for Gremlin VM in other languages.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---