You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tinkerpop.apache.org by GitBox <gi...@apache.org> on 2022/02/15 13:18:32 UTC
[GitHub] [tinkerpop] danielcweber opened a new pull request #1567: Open up 'IsDictionaryType' to recognize more actual types
danielcweber opened a new pull request #1567:
URL: https://github.com/apache/tinkerpop/pull/1567
Return true from GraphSONWriter.IsDictionaryType not only when the type is an actual Dictionary<,>, but even if it's something else that implements IEnumerable<KeyValuePair<,>>. This is useful when an instance of an ImmutableDictionary is passed as Bindings to a RequestMessage.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] FlorianHockmann commented on a change in pull request #1567: Open up 'IsDictionaryType' to recognize more actual types
Posted by GitBox <gi...@apache.org>.
FlorianHockmann commented on a change in pull request #1567:
URL: https://github.com/apache/tinkerpop/pull/1567#discussion_r807788224
##########
File path: gremlin-dotnet/src/Gremlin.Net/Structure/IO/GraphBinary/TypeSerializerRegistry.cs
##########
@@ -214,11 +212,25 @@ public ITypeSerializer GetSerializerFor(Type valueType)
throw new InvalidOperationException($"No serializer found for type ${valueType}.");
}
- private static bool IsDictionaryType(Type type)
+ private bool IsDictionaryType(Type type, out Type keyType, out Type valueType)
Review comment:
(nitpick) Why shouldn't this be `static` any more?
##########
File path: gremlin-dotnet/src/Gremlin.Net/Structure/IO/GraphBinary/TypeSerializerRegistry.cs
##########
@@ -214,11 +212,25 @@ public ITypeSerializer GetSerializerFor(Type valueType)
throw new InvalidOperationException($"No serializer found for type ${valueType}.");
}
- private static bool IsDictionaryType(Type type)
+ private bool IsDictionaryType(Type type, out Type keyType, out Type valueType)
{
- return type.IsConstructedGenericType && type.GetGenericTypeDefinition() == typeof(Dictionary<,>);
+ var maybeInterfaceType = type.GetInterfaces()
+ .FirstOrDefault(interfaceType => interfaceType.IsConstructedGenericType && interfaceType.GetGenericTypeDefinition() == typeof(IDictionary<,>));
Review comment:
`interfaceType` is also used again below in line 220. Please rename one of the two variables.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] FlorianHockmann commented on a change in pull request #1567: Open up 'IsDictionaryType' to recognize more actual types
Posted by GitBox <gi...@apache.org>.
FlorianHockmann commented on a change in pull request #1567:
URL: https://github.com/apache/tinkerpop/pull/1567#discussion_r807679638
##########
File path: gremlin-dotnet/src/Gremlin.Net/Structure/IO/GraphSON/GraphSONWriter.cs
##########
@@ -148,7 +148,9 @@ private IGraphSONSerializer TryGetSerializerFor(Type type)
private bool IsDictionaryType(Type type)
{
- return type.IsConstructedGenericType && type.GetGenericTypeDefinition() == typeof(Dictionary<,>);
+ return type
+ .GetInterfaces()
+ .Any(@interface => @interface.IsConstructedGenericType && @interface.GetGenericTypeDefinition() == typeof(IEnumerable<>) && @interface.GenericTypeArguments[0] is { IsConstructedGenericType: true } genericArgument && genericArgument.GetGenericTypeDefinition() == typeof(KeyValuePair<,>));
Review comment:
(nitpick) Please split the long line.
Also how about renaming `@interface` to `interfaceType`? I think starting a variable name with `@` is a bit uncommon and it makes me wonder as a reader whether there's anything special to it to give it this name.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] danielcweber commented on a change in pull request #1567: Open up 'IsDictionaryType' to recognize more actual types
Posted by GitBox <gi...@apache.org>.
danielcweber commented on a change in pull request #1567:
URL: https://github.com/apache/tinkerpop/pull/1567#discussion_r807827486
##########
File path: gremlin-dotnet/src/Gremlin.Net/Structure/IO/GraphBinary/TypeSerializerRegistry.cs
##########
@@ -214,11 +212,25 @@ public ITypeSerializer GetSerializerFor(Type valueType)
throw new InvalidOperationException($"No serializer found for type ${valueType}.");
}
- private static bool IsDictionaryType(Type type)
+ private bool IsDictionaryType(Type type, out Type keyType, out Type valueType)
{
- return type.IsConstructedGenericType && type.GetGenericTypeDefinition() == typeof(Dictionary<,>);
+ var maybeInterfaceType = type.GetInterfaces()
+ .FirstOrDefault(interfaceType => interfaceType.IsConstructedGenericType && interfaceType.GetGenericTypeDefinition() == typeof(IDictionary<,>));
Review comment:
I like that I can reuse names in different scopes since coming up with good names is hard. Will change it though.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] FlorianHockmann merged pull request #1567: Open up 'IsDictionaryType' to recognize more actual types
Posted by GitBox <gi...@apache.org>.
FlorianHockmann merged pull request #1567:
URL: https://github.com/apache/tinkerpop/pull/1567
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] danielcweber commented on a change in pull request #1567: Open up 'IsDictionaryType' to recognize more actual types
Posted by GitBox <gi...@apache.org>.
danielcweber commented on a change in pull request #1567:
URL: https://github.com/apache/tinkerpop/pull/1567#discussion_r807719924
##########
File path: gremlin-dotnet/src/Gremlin.Net/Structure/IO/GraphSON/GraphSONWriter.cs
##########
@@ -148,7 +148,9 @@ private IGraphSONSerializer TryGetSerializerFor(Type type)
private bool IsDictionaryType(Type type)
{
- return type.IsConstructedGenericType && type.GetGenericTypeDefinition() == typeof(Dictionary<,>);
+ return type
+ .GetInterfaces()
+ .Any(@interface => @interface.IsConstructedGenericType && @interface.GetGenericTypeDefinition() == typeof(IEnumerable<>) && @interface.GenericTypeArguments[0] is { IsConstructedGenericType: true } genericArgument && genericArgument.GetGenericTypeDefinition() == typeof(KeyValuePair<,>));
Review comment:
Done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] danielcweber commented on a change in pull request #1567: Open up 'IsDictionaryType' to recognize more actual types
Posted by GitBox <gi...@apache.org>.
danielcweber commented on a change in pull request #1567:
URL: https://github.com/apache/tinkerpop/pull/1567#discussion_r807826072
##########
File path: gremlin-dotnet/src/Gremlin.Net/Structure/IO/GraphBinary/TypeSerializerRegistry.cs
##########
@@ -214,11 +212,25 @@ public ITypeSerializer GetSerializerFor(Type valueType)
throw new InvalidOperationException($"No serializer found for type ${valueType}.");
}
- private static bool IsDictionaryType(Type type)
+ private bool IsDictionaryType(Type type, out Type keyType, out Type valueType)
Review comment:
It should...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] danielcweber edited a comment on pull request #1567: Open up 'IsDictionaryType' to recognize more actual types
Posted by GitBox <gi...@apache.org>.
danielcweber edited a comment on pull request #1567:
URL: https://github.com/apache/tinkerpop/pull/1567#issuecomment-1041312959
Implemented it (partially) for TypeSerializerRegistry. However, [this](https://github.com/apache/tinkerpop/pull/1567/files#diff-f01e183bc2441b29b7b750049765f732bc24581de050cb0095baf02fd241a94eR215) cannot run wild on an IEnumerabl<KeyValuePair<,>> because subsequently, the MapSerializer demands at least an IDictionary<,>. So that's all that can be done there I guess. Won't work with any read-only-dictionaries as it is now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] danielcweber commented on pull request #1567: Open up 'IsDictionaryType' to recognize more actual types
Posted by GitBox <gi...@apache.org>.
danielcweber commented on pull request #1567:
URL: https://github.com/apache/tinkerpop/pull/1567#issuecomment-1041312959
Implemented it (partially for TypeSerializerRegistry). However, [this](https://github.com/apache/tinkerpop/pull/1567/files#diff-f01e183bc2441b29b7b750049765f732bc24581de050cb0095baf02fd241a94eR215) cannot run wild on an IEnumerabl<KeyValuePair<,>> because subsequently, the MapSerializer demands at least an IDictionary<,>. So that's all that can be done there I guess. Won't work with any read-only-dictionaries as it is now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] danielcweber commented on pull request #1567: Open up 'IsDictionaryType' to recognize more actual types
Posted by GitBox <gi...@apache.org>.
danielcweber commented on pull request #1567:
URL: https://github.com/apache/tinkerpop/pull/1567#issuecomment-1041314271
There's even [another spot](https://github.com/apache/tinkerpop/blob/d916c5ce8baffc6b4c086608df41a8091e359b71/gremlin-dotnet/src/Gremlin.Net/Process/Traversal/Bytecode.cs#L130) that could be tackled. That would need to revert to using dynamics then, though.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org