[GitHub] tinkerpop issue #446: TINKERPOP-1483: valueMap should always return string k...

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

[GitHub] tinkerpop issue #446: TINKERPOP-1483: valueMap should always return string k...

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

[GitHub] tinkerpop issue #446: TINKERPOP-1483: valueMap should always return string k...

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

[GitHub] tinkerpop issue #446: TINKERPOP-1483: valueMap should always return string k...

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

[GitHub] tinkerpop issue #446: TINKERPOP-1483: valueMap should always return string k...

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

[GitHub] tinkerpop issue #446: TINKERPOP-1483: valueMap should always return string k...

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

[GitHub] tinkerpop issue #446: TINKERPOP-1483: valueMap should always return string k...

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

[GitHub] tinkerpop issue #446: TINKERPOP-1483: valueMap should always return string k...

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

[GitHub] tinkerpop issue #446: TINKERPOP-1483: valueMap should always return string k...

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

[GitHub] tinkerpop issue #446: TINKERPOP-1483: valueMap should always return string k...

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.

[GitHub] tinkerpop issue #446: TINKERPOP-1483: valueMap should always return string k...

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] tinkerpop issue #446: TINKERPOP-1483: valueMap should always return string k...

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

[GitHub] tinkerpop issue #446: TINKERPOP-1483: valueMap should always return string k...

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

[GitHub] tinkerpop issue #446: TINKERPOP-1483: valueMap should always return string k...

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

[GitHub] tinkerpop issue #446: TINKERPOP-1483: valueMap should always return string k...

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

[GitHub] tinkerpop issue #446: TINKERPOP-1483: valueMap should always return string k...

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

[GitHub] tinkerpop issue #446: TINKERPOP-1483: valueMap should always return string k...

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],

[GitHub] tinkerpop issue #446: TINKERPOP-1483: valueMap should always return string k...

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