You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tinkerpop.apache.org by FlorianHockmann <gi...@git.apache.org> on 2017/02/18 14:19:38 UTC
[GitHub] tinkerpop pull request #561: TINKERPOP-1635 Fix duplicate serialization of e...
GitHub user FlorianHockmann opened a pull request:
https://github.com/apache/tinkerpop/pull/561
TINKERPOP-1635 Fix duplicate serialization of element property in PropertySerializer
This fixes the duplicate serialization issue in gremlin-python's PropertySerializer:
[https://issues.apache.org/jira/browse/TINKERPOP-1635](https://issues.apache.org/jira/browse/TINKERPOP-1635)
This pull requests removes the second call to `writer.toDict()` and changes a property value in the respective unit test to an int. That makes the test fail with the duplicate serialization and pass with the fixed version.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/FlorianHockmann/tinkerpop master
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/tinkerpop/pull/561.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 #561
----
commit 4d0860b5d2f9ddf919c86ffb4e86a2eec68cb9a4
Author: Florian Hockmann <fh...@florian-hockmann.de>
Date: 2017-02-18T14:05:40Z
Fix duplicate serialization of element property
The GraphSON PropertySerializer in gremlin-python serialized the element property twice.
----
---
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] tinkerpop issue #561: TINKERPOP-1635 Fix duplicate serialization of element ...
Posted by okram <gi...@git.apache.org>.
Github user okram commented on the issue:
https://github.com/apache/tinkerpop/pull/561
Note that we will be heavily refactoring GraphSON for TinkerPop 3.3.0 (`master/`) so most of this will be gutted. I will leave it up to @spmallette / @newkek / @aholmberg to decide whether this is necessary to merge.
---
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] tinkerpop issue #561: TINKERPOP-1635 Fix duplicate serialization of element ...
Posted by davebshow <gi...@git.apache.org>.
Github user davebshow commented on the issue:
https://github.com/apache/tinkerpop/pull/561
Yeah it looks fine to me. It seems that properties are serialized before the element dict is "cleaned up". I don't see why you would serialize it again. However, I haven't really familiarized myself with the new GraphSON work on master yet. Still, I see no reason why we can't merge this. If the code is refactored and it is no longer relevant, so be it.
---
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] tinkerpop issue #561: TINKERPOP-1635 Fix duplicate serialization of element ...
Posted by okram <gi...@git.apache.org>.
Github user okram commented on the issue:
https://github.com/apache/tinkerpop/pull/561
Should this be directed to `tp32/` and not `master/`? Or is this a breaking change?
---
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] tinkerpop issue #561: TINKERPOP-1635 Fix duplicate serialization of element ...
Posted by FlorianHockmann <gi...@git.apache.org>.
Github user FlorianHockmann commented on the issue:
https://github.com/apache/tinkerpop/pull/561
Sure, I just noticed it while working on GraphSON for Gremlin.CSharp and it seemed strange that integer properties were serialized twice.
---
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] tinkerpop pull request #561: TINKERPOP-1635 Fix duplicate serialization of e...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/tinkerpop/pull/561
---
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] tinkerpop issue #561: TINKERPOP-1635 Fix duplicate serialization of element ...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on the issue:
https://github.com/apache/tinkerpop/pull/561
I'm not sure at this point what will be changed and what won't for 3.3.0 so I think it makes sense to fix stuff that isn't right even if it might change later. @davebshow does this change look ok to you?
---
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] tinkerpop issue #561: TINKERPOP-1635 Fix duplicate serialization of element ...
Posted by FlorianHockmann <gi...@git.apache.org>.
Github user FlorianHockmann commented on the issue:
https://github.com/apache/tinkerpop/pull/561
The PropertySerializer doesn't exist in the `tp32` branch, it was only added to the master. So the problem also only exists in `master`.
---
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] tinkerpop issue #561: TINKERPOP-1635 Fix duplicate serialization of element ...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on the issue:
https://github.com/apache/tinkerpop/pull/561
merged via CTR as this was a small change and there seemed to be general consensus to merge given comments.
---
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.
---