Github user dkuppitz commented on the issue:
https://github.com/apache/tinkerpop/pull/446
Looked through the code, did some manual test - all good.
VOTE: +1
Gonna merge it in a few.
---
If your project is set up for it, you can reply to this email and have your
reply
Github user dkuppitz commented on the issue:
https://github.com/apache/tinkerpop/pull/446
Hold on, I've been busy with other stuff. I will look into this PR today.
---
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 p
Github user okram commented on the issue:
https://github.com/apache/tinkerpop/pull/446
This is looking really good. We need one more VOTE.
---
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
Github user okram commented on the issue:
https://github.com/apache/tinkerpop/pull/446
I just realized that `valueMap()` is `Map`, but
`valueMap(boolean)` is `Map`. We should maintain that level of
explicitness as I suspect in the future `valueMap(boolean)` will change given
that its
Github user JPMoresmau commented on the issue:
https://github.com/apache/tinkerpop/pull/446
I've update the changelog and upgrade doc, not sure it's sufficient, please
check! thanks!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHu
Github user okram commented on the issue:
https://github.com/apache/tinkerpop/pull/446
@dkuppitz -- will you handle merge and thus, do the CHANGELOG and update
the upgrade docs on merge?
---
If your project is set up for it, you can reply to this email and have your
reply appear on G
Github user spmallette commented on the issue:
https://github.com/apache/tinkerpop/pull/446
This requires CHANGELOG and upgrade docs too (unless a committer wants to
take responsibility for that).
---
If your project is set up for it, you can reply to this email and have your
reply a
Github user okram commented on the issue:
https://github.com/apache/tinkerpop/pull/446
Yea, `Map` is the thing. :| ... I really don't like
`valueMap()`. For this reason and for the `boolean` argument overload ...
fuggly.
VOTE +1.
---
If your project is set up for it, you ca
Github user robertdale commented on the issue:
https://github.com/apache/tinkerpop/pull/446
Thanks for the clarification @dkuppitz
`./docker/build.sh -t -i -n ` passes
VOTE: +1
---
If your project is set up for it, you can reply to this email and have your
reply ap
Github user dkuppitz commented on the issue:
https://github.com/apache/tinkerpop/pull/446
> Access by: `map.get(T.id)` **or** `map.get(T.id.getAccessor())`?
By `map.get(T.id)`. Providers may allow property names that match
TinkerPop's token accessors, hence `map.get(T.id)` mus
Github user robertdale commented on the issue:
https://github.com/apache/tinkerpop/pull/446
This is interesting. I was unaware of some or didn't understand some
implementation details. Taking a step back and looking at this again, I wonder
if the original intent is more correct. Sorry
Github user robertdale commented on the issue:
https://github.com/apache/tinkerpop/pull/446
Can this PR be updated with Map or should it be closed and
open a new one?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If yo
Github user dkuppitz commented on the issue:
https://github.com/apache/tinkerpop/pull/446
Oh, now I see what you're saying. Yes, this would be the right way to go.
The current `PropertyMapStep` implementation is pretty weird.
```java
protected Map map(...) {
final
Github user JPMoresmau commented on the issue:
https://github.com/apache/tinkerpop/pull/446
What? I'm just saying that currently valueMap returns `Map`,
so if you iterate on all the keys as String, you get a runtime crash because
there are keys that are not strings. So if you don't wa
Github user dkuppitz commented on the issue:
https://github.com/apache/tinkerpop/pull/446
To support something like `valueMap(T.label, "name")`? This would be cool,
but would go into another PR.
---
If your project is set up for it, you can reply to this email and have your
reply app
Github user JPMoresmau commented on the issue:
https://github.com/apache/tinkerpop/pull/446
Fine, but if you don't want to change the implementation to match the
interface, you'll have to change the interface of valueMap... Having keys of a
type that is not allowed by the actual gener
Github user okram commented on the issue:
https://github.com/apache/tinkerpop/pull/446
Yea, this is a troublesome PR. VOTE -1.
---
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
en
Github user dkuppitz commented on the issue:
https://github.com/apache/tinkerpop/pull/446
I've seen lots of code where people actually use `id` as a property name.
```
gremlin> g = TinkerGraph.open().traversal()
==>graphtraversalsource[tinkergraph[vertices:0 edges:0], s
Github user JPMoresmau commented on the issue:
https://github.com/apache/tinkerpop/pull/446
Well, I wasn't going to change the API on my very first PR :-). The first
thing I did with a valueMap result was to iterate on the keys, which crashes...
Of course if you want to change the API
Github user okram commented on the issue:
https://github.com/apache/tinkerpop/pull/446
Hm. This is a hard pill to swallow as its not backwards compatible. I was
thinking you were going to go the route of making it `Map` and
thus, allow the enums as they are. If people have code that i
20 matches
Mail list logo
