Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
devlux opened this issue Oct 9, 2020 · 3 comments · Fixed by #54
Closed

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

devlux opened this issue Oct 9, 2020 · 3 comments · Fixed by #54

Comments

@devlux
Copy link
Contributor

devlux commented Oct 9, 2020

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

  • use the enum value (int) by default
  • use the case-sensitive name of the enum member if JsonStringEnumConverter is specified

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

as this behavior is subject to be changed in .NET5 (dotnet/runtime#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
Copy link
Collaborator

m-wild commented Oct 10, 2020

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
Copy link
Collaborator

m-wild commented Oct 10, 2020

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
Copy link
Contributor Author

devlux commented Oct 11, 2020

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants