[GitHub] tinkerpop issue #842: TINKERPOP-1945
Github user danielcweber commented on the issue: https://github.com/apache/tinkerpop/pull/842 Done! ---
[GitHub] tinkerpop issue #842: TINKERPOP-1945
Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/842 I proposed the listing on the dev list...the preview release is nice, but could you reference it in your README so that it's clear. Also, please include mention of "Apache TinkerPopâ¢" in reference to "Gremlin" - thanks. ---
[GitHub] tinkerpop issue #842: TINKERPOP-1945
Github user jorgebay commented on the issue: https://github.com/apache/tinkerpop/pull/842 Great work @danielcweber ! Thanks for your patience during the process. ---
[GitHub] tinkerpop issue #842: TINKERPOP-1945
Github user danielcweber commented on the issue: https://github.com/apache/tinkerpop/pull/842 Alright, thanks! ---
[GitHub] tinkerpop issue #842: TINKERPOP-1945
Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/842 I'm not personally in touch with anyone over there really. I usually just ping folks over there on twitter if I need to convey/ask something - try Luis Bosquez: https://twitter.com/_lbosq ---
[GitHub] tinkerpop issue #842: TINKERPOP-1945
Github user danielcweber commented on the issue: https://github.com/apache/tinkerpop/pull/842 Thanks a lot for the twitter mention! In a completely unrelated issue, is anybody of your team in contact with the graph implementors over at CosmosDB? I have no luck in getting [this issue](https://feedback.azure.com/forums/263030-azure-cosmos-db/suggestions/33998623-cosmosdb-s-implementation-of-the-tinkerpop-dsl-has) across and I wonder if there's a workaround I haven't thought of yet... ---
[GitHub] tinkerpop issue #842: TINKERPOP-1945
Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/842 nice @FlorianHockmann - i will manually close this PR now @danielcweber hope we get to see some more pull requests from you in the future. really nicely done - i bragged about your effort on twitter: https://twitter.com/spmallette/status/1029033230228447232 ---
[GitHub] tinkerpop issue #842: TINKERPOP-1945
Github user danielcweber commented on the issue: https://github.com/apache/tinkerpop/pull/842 I guess it could qualify for the listing with some minor tweaks. It doesn't have a release yet (things get deliberately broken by me all the time, so it's at most in some version 0.x, if there was a release), and, well, there is __some__ documentation... I will update the docs to reflect what it can already do today, and probably even package a preview nuget. Thanks for the offer! ---
[GitHub] tinkerpop issue #842: TINKERPOP-1945
Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/842 neat - I'd be happy to suggest adding it to our provider index on the tinkerpop home page assuming it meets the listing policy: http://tinkerpop.apache.org/policy.html please let me know if that's something you'd like to see and if so, whether or not you think you meet the policy. ---
[GitHub] tinkerpop issue #842: TINKERPOP-1945
Github user danielcweber commented on the issue: https://github.com/apache/tinkerpop/pull/842 Thanks for accepting the contribution! For what it's worth, I am developing a [type-safe ORM](https://github.com/ExRam/ExRam.Gremlinq) (abusing the 'R' in ORM a bit ;-) on top of Gremlin.net, (among others). If this is of any interest for you wrt. to the Tinkerpop repo, let me know! ---
[GitHub] tinkerpop issue #842: TINKERPOP-1945
Github user FlorianHockmann commented on the issue: https://github.com/apache/tinkerpop/pull/842 I think I found the cause of the problem: We're using `dynamic` in Gremlin.Net which requires the `Microsoft.CSharp` package. We always got that package because Newtonsoft.Json also had a dependency on it. However, Newtonsoft.Json seems to not use it anymore for .NET Standard 2.0: .NETStandard 1.3 Microsoft.CSharp (>= 4.3.0) NETStandard.Library (>= 1.6.1) System.ComponentModel.TypeConverter (>= 4.3.0) System.Runtime.Serialization.Formatters (>= 4.3.0) System.Runtime.Serialization.Primitives (>= 4.3.0) System.Xml.XmlDocument (>= 4.3.0) .NETStandard 2.0 No dependencies. So, we're now getting errors for every use of `dynamic` with .NET Standard 2.0 as the target framework. I'm now adding a direct dependency on Microsoft.CSharp to Gremlin.Net. So, this can't happen again, even if Newtonsoft.Json would get completely rid of that dependency. We already had that dependency before, it was only kind of _hidden_ behind our dependency on Newtonsoft.Json. ---
[GitHub] tinkerpop issue #842: TINKERPOP-1945
Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/842 I don't know if this matters but I noticed that the upgrade of that JSON thing didn't seem to matter to the tests. they passed on the old version (i sorta tested that version by accident because the PR updated the proj file directory and not the template so maven overwrote the change back to 9) - so do we need 9? or do we need a test case that shows we need 9? ---
[GitHub] tinkerpop issue #842: TINKERPOP-1945
Github user FlorianHockmann commented on the issue: https://github.com/apache/tinkerpop/pull/842 I'm looking into this. At a first glance it seems to be based on the combination of the upgraded Newtonsoft.Json version and the addition of the .NET Standard 2.0 target. ---
[GitHub] tinkerpop issue #842: TINKERPOP-1945
Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/842 I got the merge done on tp32, but when I merged to tp33 .NET wouldn't build. I didn't immediately recognize the problem, so perhaps it's best that @jorgebay or @FlorianHockmann pick it up from here. I've squashed/merged and added a couple of commits to this branch: https://github.com/apache/tinkerpop/commits/TINKERPOP-1945 which is based on tp32. I guess we can close this PR once we get that branch merged to tp33 and master. @danielcweber thanks again for sticking to this PR and taking it to the end. it is appreciated. ---
[GitHub] tinkerpop issue #842: TINKERPOP-1945
Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/842 I'm working on merging this now. ---
[GitHub] tinkerpop issue #842: TINKERPOP-1945
Github user jorgebay commented on the issue: https://github.com/apache/tinkerpop/pull/842 LGTM! VOTE: +1. Thanks @danielcweber for the contribution! Note to committers: I think it would be better to squash the commits ahead of the merge. ---
[GitHub] tinkerpop issue #842: TINKERPOP-1945
Github user FlorianHockmann commented on the issue: https://github.com/apache/tinkerpop/pull/842 Great, thanks for making those changes. My VOTE +1 from nearly 3 months ago still holds with these recent changes. ---
[GitHub] tinkerpop issue #842: TINKERPOP-1945
Github user danielcweber commented on the issue: https://github.com/apache/tinkerpop/pull/842 Revised based on the comments, rebased onto the current tp32 branch. ---
[GitHub] tinkerpop issue #842: TINKERPOP-1945
Github user FlorianHockmann commented on the issue: https://github.com/apache/tinkerpop/pull/842 @danielcweber the build is now failing because some of the added files miss the required license header. Could you please add that? ---
[GitHub] tinkerpop issue #842: TINKERPOP-1945
Github user danielcweber commented on the issue: https://github.com/apache/tinkerpop/pull/842 @spmallette Should be symmetrical now! ---
[GitHub] tinkerpop issue #842: TINKERPOP-1945
Github user danielcweber commented on the issue: https://github.com/apache/tinkerpop/pull/842 Sorry, I lost track of this issue since I worked around the TimeSpan issue with simple longs (which can be summed up nicely which is impossible for durations). I can delete the unneeded converters though, takes a minute. ---
[GitHub] tinkerpop issue #842: TINKERPOP-1945
Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/842 @danielcweber should we expect any change here or is this ok to close at this point? ---
[GitHub] tinkerpop issue #842: TINKERPOP-1945
Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/842 > Symmetry doesn't help me much if my GLV decides to return some obscure type that I can't process I sense the chance of that is a bit low, especially if in the future we look to discourage use of these Java specific types. Most graphs don't natively support these Java Time types, so getting one you don't expect would be odd. I think the original intent for having these at all was to better support the old script API where you might do some calculations in your Gremlin script and return one of these types, so in that case, you would be in control of whether or not you used that type in the first place. > I am available to make the appropriate changes,... Thanks, that's appreciated. > I just hope in the end there will be enough support retained for us to work with CosmosDB graph Please let us know how that pans out. ---
[GitHub] tinkerpop issue #842: TINKERPOP-1945
Github user danielcweber commented on the issue: https://github.com/apache/tinkerpop/pull/842 @spmallette I have been following the discussion. To be honest, I don't care too much about the symmetry. Symmetry doesn't help me much if my GLV decides to return some obscure type that I can't process because I also can't send it and it would break symmetry. I am just interested in Gremlin.net being able to send Java-Durations for .net-TimeSpans to the database. I am available to make the appropriate changes, which would probably just be to retrire support for the non-symmetric types. I just hope in the end there will be enough support retained for us to work with CosmosDB graph (they aren't even running on Tinkerpop). ---
[GitHub] tinkerpop issue #842: TINKERPOP-1945
Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/842 @danielcweber thanks for your patience on this pull request. Sometimes the seemingly simple tasks turn into larger more complex discussions unfortunately. I'm not sure if you've been following along as I don't recall seeing your name pop up in the previously referenced dev mailing list discussion where we tried to hash this all out, but it appears that the consensus is to maintain symmetry of serializers for .NET and to not allow one way serialization for the more richly/strongly typed GLVs. Hopefully you still are available to make the appropriate changes to help us get this merged. ---
[GitHub] tinkerpop issue #842: TINKERPOP-1945
Github user FlorianHockmann commented on the issue: https://github.com/apache/tinkerpop/pull/842 In case someone is following here but is not on the dev mailing list: https://lists.apache.org/thread.html/32c47e8682c2848a9cc90eaff0a0da1f9bf4bd5a94b746a3e143f341@%3Cdev.tinkerpop.apache.org%3E ---
[GitHub] tinkerpop issue #842: TINKERPOP-1945
Github user FlorianHockmann commented on the issue: https://github.com/apache/tinkerpop/pull/842 > @FlorianHockmann Why would we do that? If we can construct (i.e. deserialize) e.g. a DateTime of whatever a server gives us, fine. Doesn't mean we have to send (i.e. serialize) obscure types. As it currently is, it's more of an 'it just works'-approach. I think the problem Jorge sees with that is the following: 1. You receive e.g. an `gx:Instant`. 2. It gets deserialized to `DateTime`. 3. You work with this `DateTime` and then send it back to the server. 4. It will be serialized to `g:Date`. <- Can't be used where the original `gx:Instant` came from. but right, lets wait for him to come back on this. > I think the point is to establish the intent to users that we no longer intend to support a type at some point in the future. I'm not sure what deprecation means in the IO sense. Ok, since I'm also not sure about how we would want to handle the "deprecation" of types but since we seem to agree on the fact that communicating to users somehow which types they can expect to be supported across GLVs and which ones not, I'll start a DISCUSS thread on the mailing list. Maybe others have good ideas for that. ---
[GitHub] tinkerpop issue #842: TINKERPOP-1945
Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/842 I don't think it's wrong to mark something as "deprecated" in preparation for a future non-usage that may or may not happen. I think the point is to establish the intent to users that we no longer intend to support a type at some point in the future. I'm not sure what deprecation means in the IO sense. @danielcweber maybe don't act yet to just remove them all. i'd like to hear how strongly @jorgebay feels about this issue as he was the one who brought it up originally and he hasn't really had a chance to come back to this yet. depending on what he says, maybe we need a more clear DISCUSS thread on dev to make sure we have consensus for direction. as we keep wobbling back/forth here a bit. ---
[GitHub] tinkerpop issue #842: TINKERPOP-1945
Github user danielcweber commented on the issue: https://github.com/apache/tinkerpop/pull/842 >remove all deserializers again for which no serializer exists and vice versa Why would we do that? If we can construct (i.e. deserialize) e.g. a DateTime of whatever a server gives us, fine. Doesn't mean we have to send (i.e. serialize) obscure types. As it currently is, it's more of a 'it just works'-approach. ---
[GitHub] tinkerpop issue #842: TINKERPOP-1945
Github user FlorianHockmann commented on the issue: https://github.com/apache/tinkerpop/pull/842 After thinking a bit more about this, my conclusion was that deprecating those types doesn't really make sense unless we plan on GraphSON 4.0 as we couldn't remove the types before that. Since we already have those types since GraphSON 2.0 and they can only be used when the extended module is explicitly selected I guess that we don't have to take immediate action at the moment. Usage of the extended types is probably relatively low in general anyway. But for the new serialization format @jorgebay suggested we should really check which types we want to support at all. I added a comment to the ticket so we don't forget that aspect. So @danielcweber, looks like it would be best if you could remove all deserializers again for which no serializer exists and vice versa. Sorry for the unnecessary work you put into this. ---
[GitHub] tinkerpop issue #842: TINKERPOP-1945
Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/842 > Could we then somehow mark types as deprecated that we identify as problematic for non-Java languages? Not sure, but feel free to start fire up a DISCUSS thread on the mailing list about that if you like. Starting to talk about deprecation of types is an interesting idea to start polishing up for a core set of types for @jorgebay and https://issues.apache.org/jira/browse/TINKERPOP-1942 ---
[GitHub] tinkerpop issue #842: TINKERPOP-1945
Github user FlorianHockmann commented on the issue: https://github.com/apache/tinkerpop/pull/842 I'm ok with either direction - either to only support types symmetrically or try to support at the reading site as much types as possible - as long as we make that decision clear and follow it consistently for all GLVs. Could we then somehow mark types as deprecated that we identify as problematic for non-Java languages? That would make it clear to users that they can expect types like `byte` or `char` to be supported across GLVs, but that they should avoid using types like `Instant` or `YearMonth`. Otherwise users basically have to check the source code of the GLV(s) they're using when they want to know which type they can use and even if it's not implemented they can't know whether it was omitted on purpose or if the implementation is just still missing. ---
[GitHub] tinkerpop issue #842: TINKERPOP-1945
Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/842 We probably should never have started support for those Java specific duration types. dah - stupid exotic types. anyway... There is no precedence for these extended types in GLVs i don't think so we should probably be careful about this choice from now. If we can't get symmetry in a particular language, perhaps we should avoid support in the first place, at least for now. I say "at least for now" because it's easy to make it asymmetric later if we want, but the reverse of that is not so easy. Thoughts? ---
[GitHub] tinkerpop issue #842: TINKERPOP-1945
Github user danielcweber commented on the issue: https://github.com/apache/tinkerpop/pull/842 I don't know how to get total symmetry into that. As long as Tinkerpop I/O-docs don't explain the important difference between e.g. a Date and an Instant, and as long as there are vastly more time-related structures on the Java side than there are on the .net side, we can't do better than this I guess. ---
[GitHub] tinkerpop issue #842: TINKERPOP-1945
Github user jorgebay commented on the issue: https://github.com/apache/tinkerpop/pull/842 Sorry I'm late to the party, I haven't got much time during the past weeks: nice contribution @danielcweber ! I'm a little concerned about the difference between serializers and deserializers supported. Can we limit the scope of this PR to types for which we support serialization and deserialization? Some of the extended types are not very popular but once we add support for them, the user base for those could grow. From the user perspective, retrieving and storing are expected to be symmetric: if one operation is supported the other should also be. Mapping 1 type (like `DateTime`) to multiple TinkerPop types could lead to obscure serialization errors that the user might not be able to identify. ---
