You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tinkerpop.apache.org by jorgebay <gi...@git.apache.org> on 2017/11/22 11:17:34 UTC
[GitHub] tinkerpop pull request #757: TINKERPOP-1837 Gremlin .NET: Provide type coerc...
GitHub user jorgebay opened a pull request:
https://github.com/apache/tinkerpop/pull/757
TINKERPOP-1837 Gremlin .NET: Provide type coercion between IDictionary<K, V> instances
https://issues.apache.org/jira/browse/TINKERPOP-1837
(The new Travis job comes in handy to validate features like this one :) )
VOTE +1
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/apache/tinkerpop TINKERPOP-1837
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/tinkerpop/pull/757.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 #757
----
commit a895a6847521a8aa30a83344cf3262aaa1d8c22f
Author: Jorge Bay Gondra <jo...@gmail.com>
Date: 2017-11-22T10:44:00Z
Gremlin .NET: Provide type coercion between IDictionary<K, V> instances
----
---
[GitHub] tinkerpop pull request #757: TINKERPOP-1837 Gremlin .NET: Provide type coerc...
Posted by jorgebay <gi...@git.apache.org>.
Github user jorgebay commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/757#discussion_r153128066
--- Diff: gremlin-dotnet/src/Gremlin.Net/Process/Traversal/DefaultTraversal.cs ---
@@ -86,9 +87,61 @@ public void Reset()
}
/// <inheritdoc />
- public E Current => (E)TraverserEnumerator.Current?.Object;
+ public E Current => GetCurrent<E>();
- object IEnumerator.Current => Current;
+ object IEnumerator.Current => GetCurrent();
+
+ private TReturn GetCurrent<TReturn>()
+ {
+ var value = GetCurrent();
+ var returnType = typeof(TReturn);
+ if (value == null || value.GetType() == returnType)
+ {
+ // Avoid evaluating type comparisons
+ return (TReturn) value;
+ }
+ return (TReturn) GetValue(returnType, value);
+ }
+
+ private object GetCurrent()
+ {
+ // Use dynamic to object to prevent runtime dynamic conversion evaluation
+ return TraverserEnumerator.Current?.Object;
+ }
+
+ /// <summary>
+ /// Gets the value, converting to the expected type when necessary and supported.
+ /// </summary>
+ private static object GetValue(Type type, object value)
+ {
+ var genericType = type.GetTypeInfo().IsGenericType
+ ? type.GetTypeInfo().GetGenericTypeDefinition()
+ : null;
+ if (value is IDictionary dictValue && genericType == typeof(IDictionary<,>))
+ {
+ var keyType = type.GenericTypeArguments[0];
+ var valueType = type.GenericTypeArguments[1];
+ var mapType = typeof(Dictionary<,>).MakeGenericType(keyType, valueType);
+ var result = (IDictionary) Activator.CreateInstance(mapType);
+ foreach (DictionaryEntry kv in dictValue)
+ {
+ result.Add(GetValue(keyType, kv.Key), GetValue(valueType, kv.Value));
+ }
+ return result;
+ }
+ if (value is IEnumerable enumerableValue && genericType == typeof(IList<>))
--- End diff --
Its always easier to go from an strict API to a wider range of representations in the future than the other way around.
---
[GitHub] tinkerpop issue #757: TINKERPOP-1837 Gremlin .NET: Provide type coercion bet...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on the issue:
https://github.com/apache/tinkerpop/pull/757
VOTE +1
---
[GitHub] tinkerpop pull request #757: TINKERPOP-1837 Gremlin .NET: Provide type coerc...
Posted by FlorianHockmann <gi...@git.apache.org>.
Github user FlorianHockmann commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/757#discussion_r153068981
--- Diff: gremlin-dotnet/src/Gremlin.Net/Process/Traversal/DefaultTraversal.cs ---
@@ -86,9 +87,61 @@ public void Reset()
}
/// <inheritdoc />
- public E Current => (E)TraverserEnumerator.Current?.Object;
+ public E Current => GetCurrent<E>();
- object IEnumerator.Current => Current;
+ object IEnumerator.Current => GetCurrent();
+
+ private TReturn GetCurrent<TReturn>()
+ {
+ var value = GetCurrent();
+ var returnType = typeof(TReturn);
+ if (value == null || value.GetType() == returnType)
+ {
+ // Avoid evaluating type comparisons
+ return (TReturn) value;
+ }
+ return (TReturn) GetValue(returnType, value);
+ }
+
+ private object GetCurrent()
+ {
+ // Use dynamic to object to prevent runtime dynamic conversion evaluation
+ return TraverserEnumerator.Current?.Object;
+ }
+
+ /// <summary>
+ /// Gets the value, converting to the expected type when necessary and supported.
+ /// </summary>
+ private static object GetValue(Type type, object value)
+ {
+ var genericType = type.GetTypeInfo().IsGenericType
+ ? type.GetTypeInfo().GetGenericTypeDefinition()
+ : null;
+ if (value is IDictionary dictValue && genericType == typeof(IDictionary<,>))
+ {
+ var keyType = type.GenericTypeArguments[0];
+ var valueType = type.GenericTypeArguments[1];
+ var mapType = typeof(Dictionary<,>).MakeGenericType(keyType, valueType);
+ var result = (IDictionary) Activator.CreateInstance(mapType);
+ foreach (DictionaryEntry kv in dictValue)
+ {
+ result.Add(GetValue(keyType, kv.Key), GetValue(valueType, kv.Value));
+ }
+ return result;
+ }
+ if (value is IEnumerable enumerableValue && genericType == typeof(IList<>))
--- End diff --
Can't we change this to also allow `List<>` as the `genericType` instead of `IList<>`?
---
[GitHub] tinkerpop pull request #757: TINKERPOP-1837 Gremlin .NET: Provide type coerc...
Posted by jorgebay <gi...@git.apache.org>.
Github user jorgebay commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/757#discussion_r153151298
--- Diff: gremlin-dotnet/src/Gremlin.Net/Process/Traversal/DefaultTraversal.cs ---
@@ -86,9 +87,61 @@ public void Reset()
}
/// <inheritdoc />
- public E Current => (E)TraverserEnumerator.Current?.Object;
+ public E Current => GetCurrent<E>();
- object IEnumerator.Current => Current;
+ object IEnumerator.Current => GetCurrent();
+
+ private TReturn GetCurrent<TReturn>()
+ {
+ var value = GetCurrent();
+ var returnType = typeof(TReturn);
+ if (value == null || value.GetType() == returnType)
+ {
+ // Avoid evaluating type comparisons
+ return (TReturn) value;
+ }
+ return (TReturn) GetValue(returnType, value);
+ }
+
+ private object GetCurrent()
+ {
+ // Use dynamic to object to prevent runtime dynamic conversion evaluation
+ return TraverserEnumerator.Current?.Object;
+ }
+
+ /// <summary>
+ /// Gets the value, converting to the expected type when necessary and supported.
+ /// </summary>
+ private static object GetValue(Type type, object value)
+ {
+ var genericType = type.GetTypeInfo().IsGenericType
+ ? type.GetTypeInfo().GetGenericTypeDefinition()
+ : null;
+ if (value is IDictionary dictValue && genericType == typeof(IDictionary<,>))
+ {
+ var keyType = type.GenericTypeArguments[0];
+ var valueType = type.GenericTypeArguments[1];
+ var mapType = typeof(Dictionary<,>).MakeGenericType(keyType, valueType);
+ var result = (IDictionary) Activator.CreateInstance(mapType);
+ foreach (DictionaryEntry kv in dictValue)
+ {
+ result.Add(GetValue(keyType, kv.Key), GetValue(valueType, kv.Value));
+ }
+ return result;
+ }
+ if (value is IEnumerable enumerableValue && genericType == typeof(IList<>))
--- End diff --
The conversion mechanism as implemented will only work for `IDictionary<K, V>` and `IList<T>`, which are the two types for which the serialization has a grey area: maps are deserialized as `IDictionary<object, object>` and lists as `IList<object>`.
Additional conversion mechanisms, like `IList<object>` to `List<T>` (class), is not required to provide a comfortable API, so we can leave it out, at least for now.
---
[GitHub] tinkerpop pull request #757: TINKERPOP-1837 Gremlin .NET: Provide type coerc...
Posted by jorgebay <gi...@git.apache.org>.
Github user jorgebay commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/757#discussion_r153125359
--- Diff: gremlin-dotnet/src/Gremlin.Net/Process/Traversal/DefaultTraversal.cs ---
@@ -86,9 +87,61 @@ public void Reset()
}
/// <inheritdoc />
- public E Current => (E)TraverserEnumerator.Current?.Object;
+ public E Current => GetCurrent<E>();
- object IEnumerator.Current => Current;
+ object IEnumerator.Current => GetCurrent();
+
+ private TReturn GetCurrent<TReturn>()
+ {
+ var value = GetCurrent();
+ var returnType = typeof(TReturn);
+ if (value == null || value.GetType() == returnType)
+ {
+ // Avoid evaluating type comparisons
+ return (TReturn) value;
+ }
+ return (TReturn) GetValue(returnType, value);
+ }
+
+ private object GetCurrent()
+ {
+ // Use dynamic to object to prevent runtime dynamic conversion evaluation
+ return TraverserEnumerator.Current?.Object;
+ }
+
+ /// <summary>
+ /// Gets the value, converting to the expected type when necessary and supported.
+ /// </summary>
+ private static object GetValue(Type type, object value)
+ {
+ var genericType = type.GetTypeInfo().IsGenericType
+ ? type.GetTypeInfo().GetGenericTypeDefinition()
+ : null;
+ if (value is IDictionary dictValue && genericType == typeof(IDictionary<,>))
+ {
+ var keyType = type.GenericTypeArguments[0];
+ var valueType = type.GenericTypeArguments[1];
+ var mapType = typeof(Dictionary<,>).MakeGenericType(keyType, valueType);
+ var result = (IDictionary) Activator.CreateInstance(mapType);
+ foreach (DictionaryEntry kv in dictValue)
+ {
+ result.Add(GetValue(keyType, kv.Key), GetValue(valueType, kv.Value));
+ }
+ return result;
+ }
+ if (value is IEnumerable enumerableValue && genericType == typeof(IList<>))
--- End diff --
I think continue using the interfaces as the return data type allow us to hide implementation details and its an accurate representation of the Java types (`Map<,>` and `List<>` are interfaces in java).
---
[GitHub] tinkerpop pull request #757: TINKERPOP-1837 Gremlin .NET: Provide type coerc...
Posted by FlorianHockmann <gi...@git.apache.org>.
Github user FlorianHockmann commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/757#discussion_r153148019
--- Diff: gremlin-dotnet/src/Gremlin.Net/Process/Traversal/DefaultTraversal.cs ---
@@ -86,9 +87,61 @@ public void Reset()
}
/// <inheritdoc />
- public E Current => (E)TraverserEnumerator.Current?.Object;
+ public E Current => GetCurrent<E>();
- object IEnumerator.Current => Current;
+ object IEnumerator.Current => GetCurrent();
+
+ private TReturn GetCurrent<TReturn>()
+ {
+ var value = GetCurrent();
+ var returnType = typeof(TReturn);
+ if (value == null || value.GetType() == returnType)
+ {
+ // Avoid evaluating type comparisons
+ return (TReturn) value;
+ }
+ return (TReturn) GetValue(returnType, value);
+ }
+
+ private object GetCurrent()
+ {
+ // Use dynamic to object to prevent runtime dynamic conversion evaluation
+ return TraverserEnumerator.Current?.Object;
+ }
+
+ /// <summary>
+ /// Gets the value, converting to the expected type when necessary and supported.
+ /// </summary>
+ private static object GetValue(Type type, object value)
+ {
+ var genericType = type.GetTypeInfo().IsGenericType
+ ? type.GetTypeInfo().GetGenericTypeDefinition()
+ : null;
+ if (value is IDictionary dictValue && genericType == typeof(IDictionary<,>))
+ {
+ var keyType = type.GenericTypeArguments[0];
+ var valueType = type.GenericTypeArguments[1];
+ var mapType = typeof(Dictionary<,>).MakeGenericType(keyType, valueType);
+ var result = (IDictionary) Activator.CreateInstance(mapType);
+ foreach (DictionaryEntry kv in dictValue)
+ {
+ result.Add(GetValue(keyType, kv.Key), GetValue(valueType, kv.Value));
+ }
+ return result;
+ }
+ if (value is IEnumerable enumerableValue && genericType == typeof(IList<>))
--- End diff --
I think we are talking about different things here. I'm not talking about the API, but about the strict condition of this `if` clause that expects a specified type of `IList<>`.
However, as a user I can execute a traversal like `g.V().ValueMap<List<int>>("age").Next()` which results in a `genericType` of `List<>` here instead of `IList<>`.
Shouldn't we also support something like that where the user doesn't specify `IList` as the return type? And if not then we should probably tell users somehow that they should always use `IList` in cases like this and not `List`.
---
[GitHub] tinkerpop pull request #757: TINKERPOP-1837 Gremlin .NET: Provide type coerc...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/tinkerpop/pull/757
---
[GitHub] tinkerpop pull request #757: TINKERPOP-1837 Gremlin .NET: Provide type coerc...
Posted by FlorianHockmann <gi...@git.apache.org>.
Github user FlorianHockmann commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/757#discussion_r153126228
--- Diff: gremlin-dotnet/src/Gremlin.Net/Process/Traversal/DefaultTraversal.cs ---
@@ -86,9 +87,61 @@ public void Reset()
}
/// <inheritdoc />
- public E Current => (E)TraverserEnumerator.Current?.Object;
+ public E Current => GetCurrent<E>();
- object IEnumerator.Current => Current;
+ object IEnumerator.Current => GetCurrent();
+
+ private TReturn GetCurrent<TReturn>()
+ {
+ var value = GetCurrent();
+ var returnType = typeof(TReturn);
+ if (value == null || value.GetType() == returnType)
+ {
+ // Avoid evaluating type comparisons
+ return (TReturn) value;
+ }
+ return (TReturn) GetValue(returnType, value);
+ }
+
+ private object GetCurrent()
+ {
+ // Use dynamic to object to prevent runtime dynamic conversion evaluation
+ return TraverserEnumerator.Current?.Object;
+ }
+
+ /// <summary>
+ /// Gets the value, converting to the expected type when necessary and supported.
+ /// </summary>
+ private static object GetValue(Type type, object value)
+ {
+ var genericType = type.GetTypeInfo().IsGenericType
+ ? type.GetTypeInfo().GetGenericTypeDefinition()
+ : null;
+ if (value is IDictionary dictValue && genericType == typeof(IDictionary<,>))
+ {
+ var keyType = type.GenericTypeArguments[0];
+ var valueType = type.GenericTypeArguments[1];
+ var mapType = typeof(Dictionary<,>).MakeGenericType(keyType, valueType);
+ var result = (IDictionary) Activator.CreateInstance(mapType);
+ foreach (DictionaryEntry kv in dictValue)
+ {
+ result.Add(GetValue(keyType, kv.Key), GetValue(valueType, kv.Value));
+ }
+ return result;
+ }
+ if (value is IEnumerable enumerableValue && genericType == typeof(IList<>))
--- End diff --
I don't want to change the return types for the traversal API, but a user can specify the type for some steps directly like you did in the added tests: `ValueMap<IList<int>>`. When `ValueMap<List<int>>` is used instead then it won't work right now.
---
[GitHub] tinkerpop issue #757: TINKERPOP-1837 Gremlin .NET: Provide type coercion bet...
Posted by FlorianHockmann <gi...@git.apache.org>.
Github user FlorianHockmann commented on the issue:
https://github.com/apache/tinkerpop/pull/757
The changes look good to me.
VOTE +1
---
[GitHub] tinkerpop pull request #757: TINKERPOP-1837 Gremlin .NET: Provide type coerc...
Posted by FlorianHockmann <gi...@git.apache.org>.
Github user FlorianHockmann commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/757#discussion_r153193506
--- Diff: gremlin-dotnet/src/Gremlin.Net/Process/Traversal/DefaultTraversal.cs ---
@@ -86,9 +87,61 @@ public void Reset()
}
/// <inheritdoc />
- public E Current => (E)TraverserEnumerator.Current?.Object;
+ public E Current => GetCurrent<E>();
- object IEnumerator.Current => Current;
+ object IEnumerator.Current => GetCurrent();
+
+ private TReturn GetCurrent<TReturn>()
+ {
+ var value = GetCurrent();
+ var returnType = typeof(TReturn);
+ if (value == null || value.GetType() == returnType)
+ {
+ // Avoid evaluating type comparisons
+ return (TReturn) value;
+ }
+ return (TReturn) GetValue(returnType, value);
+ }
+
+ private object GetCurrent()
+ {
+ // Use dynamic to object to prevent runtime dynamic conversion evaluation
+ return TraverserEnumerator.Current?.Object;
+ }
+
+ /// <summary>
+ /// Gets the value, converting to the expected type when necessary and supported.
+ /// </summary>
+ private static object GetValue(Type type, object value)
+ {
+ var genericType = type.GetTypeInfo().IsGenericType
+ ? type.GetTypeInfo().GetGenericTypeDefinition()
+ : null;
+ if (value is IDictionary dictValue && genericType == typeof(IDictionary<,>))
+ {
+ var keyType = type.GenericTypeArguments[0];
+ var valueType = type.GenericTypeArguments[1];
+ var mapType = typeof(Dictionary<,>).MakeGenericType(keyType, valueType);
+ var result = (IDictionary) Activator.CreateInstance(mapType);
+ foreach (DictionaryEntry kv in dictValue)
+ {
+ result.Add(GetValue(keyType, kv.Key), GetValue(valueType, kv.Value));
+ }
+ return result;
+ }
+ if (value is IEnumerable enumerableValue && genericType == typeof(IList<>))
--- End diff --
Ok, then lets leave it as it is in this PR. We can still change it in case it leads to problems for users.
---