CSHARP-2096: Make EnumRepresentationConvention also affect collection… #1535
Conversation
|
This is still in draft because I need to check what happens with dictionaries |
| } | ||
| } | ||
|
|
||
| // private methods | ||
| private bool IsNullableEnum(Type type) | ||
| private IBsonSerializer Reconfigure(IBsonSerializer serializer) |
There was a problem hiding this comment.
This method here is recursively trying to fin an EnumSerializer. If it finds it, it changes its representation and sets it as the new child serializer.
If no EnumSerializer "leaf" is found, it will return null.
There was a problem hiding this comment.
Searching for an EnumSerializer is a little stricter than what we used to do, which was search for a serializer for an enum value type that was representation configurable.
There was a problem hiding this comment.
Exactly, and this is also why I think it would make sense to do this for next major version, as this is effectively a behaviour breaking change. What do you think?
|
|
||
| if (memberTypeInfo.IsEnum) | ||
| if (memberMap.MemberType.IsEnum && serializer is IRepresentationConfigurable representationConfigurableSerializer) |
There was a problem hiding this comment.
This part here of the code could be removed, as it is a special case of Reconfigure. I decided to keep it as maybe it's slightly faster than Reconfigure and it's possibly a much more common case.
I've removed the "nullable enum" case though, but we can decide it needs to have the same treatment as this.
| @@ -14,8 +14,7 @@ | |||
| */ | |||
There was a problem hiding this comment.
This is effectively a beahviour breaking change. Should we keep this for next major version?
| } | ||
|
|
||
| var serializerType = serializer.GetType(); | ||
| if (serializerType.IsGenericType && serializerType.GetGenericTypeDefinition() == typeof(EnumSerializer<>)) |
There was a problem hiding this comment.
I'm not sure if there is an easier/more efficient way of doing this here.
There was a problem hiding this comment.
How about?
if (serializer.ValueType.IsEnum && serializer is IRepresentationConfigurable representationConfigurable)
{
return representationConfigurable.WithRepresentation(_representation);
}
There was a problem hiding this comment.
That's perfect! I did not think about this 🤦
| return; | ||
| } | ||
|
|
||
| if (IsNullableEnum(memberType)) | ||
| var reconfiguredSerializer = Reconfigure(serializer); | ||
| if (reconfiguredSerializer is not null) |
There was a problem hiding this comment.
Why do you prefer is not null to != null?
There was a problem hiding this comment.
I think sometimes it reads a little bit better, but in this case it probably does not make much of a difference. So I can definitely use != if we prefer it
| } | ||
| } | ||
|
|
||
| // private methods | ||
| private bool IsNullableEnum(Type type) | ||
| private IBsonSerializer Reconfigure(IBsonSerializer serializer) |
There was a problem hiding this comment.
Searching for an EnumSerializer is a little stricter than what we used to do, which was search for a serializer for an enum value type that was representation configurable.
| } | ||
|
|
||
| var serializerType = serializer.GetType(); | ||
| if (serializerType.IsGenericType && serializerType.GetGenericTypeDefinition() == typeof(EnumSerializer<>)) |
There was a problem hiding this comment.
How about?
if (serializer.ValueType.IsEnum && serializer is IRepresentationConfigurable representationConfigurable)
{
return representationConfigurable.WithRepresentation(_representation);
}
…s of Enums