asyncapi / saunter

Saunter is a code-first AsyncAPI documentation generator for dotnet.
https://www.asyncapi.com/
MIT License
201 stars 56 forks source link

align the enum name handling with the behavior of system.text.json #53

Closed devlux closed 3 years ago

devlux commented 3 years ago

align the enum handling with the behavior of System.Text.Json:

other than that, use the EnumMemberAttribute if the internal EnumMemberConverter is specified

as this behavior is subject to be changed in .NET5 (https://github.com/dotnet/runtime/issues/31081) I would propose to add support for the JsonStringEnumMemberConverter (part of Macross.Json.Extensions) by applying the same logic as with the internal EnumMemberConverter.

m-wild commented 3 years ago

I don't see this feature supported as at 5.0.100-rc.1.20452.10...

The PR is mostly fine, but I have a couple of concerns:

  1. Using the specific Macross.Json.Extensions package will add this as a dependency for all users. But this does seem to be quite a common solution to this problem.
  2. I'd prefer to see what the official support in .NET5 is
  3. We could remove the internal EnumMemberConverter as this is essentially doing the same thing.
m-wild commented 3 years ago

Looking through some of the PRs to donet/runtime & dotnet/corefx for the linked issue, I came across this one https://github.com/dotnet/corefx/pull/41648/files which uses reflection with the string names of types and attributes.

If we applied the same process within this library we could support the Macross.Json.Extensions-provided JsonStringEnumMemberConverter, and also support all of the Newtonsoft.Json attributes, without needing to add either as a dependency.

Attribute enumMemberAttribute = field.GetCustomAttributes()?.FirstOrDefault(ca => ca.GetType().FullName == "System.Runtime.Serialization.EnumMemberAttribute");
if (enumMemberAttribute != null)
{
    transformedName = GetEnumMemberValue(enumMemberAttribute) ?? name;
}

...

private static string GetEnumMemberValue(Attribute enumMemberAttribute)
{
    var enumMemberAttributeValuePropertyInfo = enumMemberAttribute
            .GetType()
            .GetProperty("Value", BindingFlags.Public | BindingFlags.Instance);

    return (string)enumMemberAttributeValuePropertyInfo.GetValue(enumMemberAttribute);
}

This would also solve #48 without needing a separate package.

devlux commented 3 years ago

I think the latter approach makes more sense for the JsonStringEnumConverter - at least as a workaround. As I'm still not very convinced by the magic string handling, I think we we should try to minimize the use as much as possible :-). But I would definitely not re-use this strategy for the newtonsoft related part, as there are at least two more things to consider: the naming policy is applied differently and it uses a different ignore attribute. IMHO this would really add to much clutter. On the other hand a small dedicated package would separate the different strategies and keep the code more concise.