[GitHub] tinkerpop issue #448: Python glv graphson update
Github user okram commented on the issue: https://github.com/apache/tinkerpop/pull/448 Hi @al3xandru This work has already been merged and will be released in TinkerPop 3.2.3. To answer your concerns though: 1. In order to make life easier for TinkerPop, its important that we keep the same conventions throughout all the Gremlin language variants. If there is `GraphSONReader/Writer` in Java, `GraphSONIO` in Python, and `GraphSONifier` in JavaScript, it will become increasingly difficult for us to know what does what. Hence, we made a strong stance to say -- "keep the naming the same." With that, I don't see why a `GraphSONIO` utility class couldn't be created that simply references the `GraphSONReader/Writer` methods. 2. We want idiomatic in some spots and not so in others. Backend utility classes -- non-idomatic (lets do our best to be "the same" across all GLVs). Front end user classes --- idiomatic. For example, Gremlin-Ruby would be `g.V().out_e(:knows).weight` ... --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] tinkerpop issue #448: Python glv graphson update
Github user al3xandru commented on the issue: https://github.com/apache/tinkerpop/pull/448 A bit late to the conversation as I've noticed this hasn't been merged yet. > I don't think there's a use case in the context of the python GLV, but we have that use case in GraphSON in legacy support for TinkerPop 2.x based GraphSON (the migration scenario mentioned). I don't know that we'd extend that to Python. Considering that the GraphSONIO provides both a simpler API and easier integration as noted by @davebshow, plus it doesn't need to account for the legacy support, there are only benefits with this approach. > I guess the point here is to try to maintain API consistency across all the languages, even if we don't quite have all the support for all the features from Java applicable to Python just yet. That's indeed a very good point. Assuming the main users of this API are the GLV implementors, then I'm wondering if making it idiomatic for the target platform shouldn't be the top goal. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] tinkerpop issue #448: Python glv graphson update
Github user aholmberg commented on the issue: https://github.com/apache/tinkerpop/pull/448 Pushed the split names. I didn't see issues related to these changes running `process-docs.sh`. May have been related to the previous removal of GraphSONWriter/Reader. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] tinkerpop issue #448: Python glv graphson update
Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/448 > Maybe we should clarify what APIs really need to be preserved. imo i think that could be debated. at this point, i think being sorta cautious and following what's been working for a java though seems prudent. thanks for re-working. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] tinkerpop issue #448: Python glv graphson update
Github user aholmberg commented on the issue: https://github.com/apache/tinkerpop/pull/448 Maybe we should clarify what APIs really need to be preserved. I have been thinking about the `process` package, and `structure` module as the Gremlin API, and the `driver` and `io` as distinct integrations. I'm working on splitting reader/writer now, but I'm not convinced this API is worth replicating here. We can chalk it up to my ignorance of the greater ecosystem machinery. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] tinkerpop issue #448: Python glv graphson update
Github user okram commented on the issue: https://github.com/apache/tinkerpop/pull/448 @aholmberg We have had a distinction between reader/writer since the beginning of TinkerPop. I believe (@spmallette knows more) that there is a `GryoMapper` class that has all the serializers registered. Thus, both reader and writer have reference to a `GryoMapper`. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] tinkerpop issue #448: Python glv graphson update
Github user aholmberg commented on the issue: https://github.com/apache/tinkerpop/pull/448 Thanks. I see the code there. What is not clear to me is why these two things are divided, and why that API should be replicated here. I'm having a hard time doing this change because it makes it more cumbersome to use (at least for the integration I'm considering). I can vaguely imagine using readers and writers in a migration scenario where input and output are different. Can you help me understand the use case in the context of this GLV, where client IO uses a single format? --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] tinkerpop issue #448: Python glv graphson update
Github user okram commented on the issue: https://github.com/apache/tinkerpop/pull/448 Cool @aholmberg ... Here are some more pointers. https://github.com/apache/tinkerpop/blob/master/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/io/GraphReader.java https://github.com/apache/tinkerpop/blob/master/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/io/GraphWriter.java Where `readObject/writeObject` are the genero methods. Also, you might need to update `init-code-blocks.awk` so that the docs build. https://github.com/apache/tinkerpop/blob/master/docs/preprocessor/awk/init-code-blocks.awk#L83 To test it -- `bin/process-docs.sh -f docs/src/reference/gremlin-variants.asciidoc`. If you can't test it easily, no worries. I will fiddle the `awk` and testing of doc building for 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] tinkerpop issue #448: Python glv graphson update
Github user aholmberg commented on the issue: https://github.com/apache/tinkerpop/pull/448 (also, ACK on rebase -- I should be getting to it 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 project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] tinkerpop issue #448: Python glv graphson update
Github user aholmberg commented on the issue: https://github.com/apache/tinkerpop/pull/448 > you changed GraphSONWriter and GraphSONWriter to GraphSONIO This was not Python-specific, but felt like a natural design to me. I unified the I/O per type, and tied those together at the top level in `GraphSONIO`. It seems strange to me to have the two sides of the same encoding split between classes. I thought I saw something in the java impl called `GraphSONIO`, but maybe I was mistaken. I can split them up, but then it means carrying around two things to encode/decode everywhere, instead of one: `connection = DriverConnection(..., graphson_reader=GraphSONReader(...), graphson_writer=GraphSONWriter(...))` vs. `connection = DriverConnection(..., graphson_io=GraphSONIO(...))` I'll study the Java impl. a little closer to see if I can understand the benefit. Please let me know if you have any further thoughts. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] tinkerpop issue #448: Python glv graphson update
Github user okram commented on the issue: https://github.com/apache/tinkerpop/pull/448 Hi -- one thing to add. I notice you changed `GraphSONWriter` and `GraphSONWriter` to `GraphSONIO` ... The writer/reader names are identical to Gremlin-Java. If its possible, can we leave the original class names? .. unless there is a strong pattern in Python that says that is bad. Thanks. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] tinkerpop issue #448: Python glv graphson update
Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/448 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 enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] tinkerpop issue #448: Python glv graphson update
Github user okram commented on the issue: https://github.com/apache/tinkerpop/pull/448 I like these changes. With a solid rebase of master/ given my recent `TraversalStrategy` serializer work, I'm good with merging this 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 enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] tinkerpop issue #448: Python glv graphson update
Github user davebshow commented on the issue: https://github.com/apache/tinkerpop/pull/448 Yeah makes sense. I'm hoping to do some major refactors of the Driver over the next couple months anyway, but I'm juggling too many things right now. Either way this seems good to go. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] tinkerpop issue #448: Python glv graphson update
Github user okram commented on the issue: https://github.com/apache/tinkerpop/pull/448 @aholmberg I know. Sorry. I'm not graceful. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] tinkerpop issue #448: Python glv graphson update
Github user aholmberg commented on the issue: https://github.com/apache/tinkerpop/pull/448 argh. needs a third rebase, too :-/ --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] tinkerpop issue #448: Python glv graphson update
Github user aholmberg commented on the issue: https://github.com/apache/tinkerpop/pull/448 > graphson_io=None to the __init__ method signature I think that is a fine idea. I was mostly focused on the GraphSON API and not concerned with adding it to the token RemoteConnection implementation (figuring it would get pulled in if someone saw cause to do it). If you think it's worth doing now (and it won't invalidate this vote), I can push the update I have here. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] tinkerpop issue #448: Python glv graphson update
Github user aholmberg commented on the issue: https://github.com/apache/tinkerpop/pull/448 I'm ambivalent on that. I just noticed unittest was present, and the bare assertions were not very informative. I can take that commit out if you think it's not worth it now. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] tinkerpop issue #448: Python glv graphson update
Github user okram commented on the issue: https://github.com/apache/tinkerpop/pull/448 This looks good. However, can you please rebase with `master/` as there have been some changes to `graphson.py`. Also, if there is anything that needs to be added to `gremlin-variants.asciidoc` about registering serializers/deserializers, please add. Thanks. 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 enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
