Handle DisplayAttribute.

[adamradocz]

Closes #11

Handles the [Display(Name = "Customer name")] attribute. It works on .NET Core 3.1+, but unfortunately, I couldn't figure out why doesn't it work on .NET Framework 4.8.

[andrewlock]

This is good work, thanks! Before we merge it, I think we should do 2 things:

1. Add an integration test for the behaviour
2. Figure out why it doesn't work in .NET Framework

Definitely happy to merge it once we have those, thanks again!

[FroggieFrog]

Edit:
I just found out a simpler way:

1. Add package System.ComponentModel.Annotations to NetEscapades.EnumGenerators.Tests and NetEscapades.EnumGenerators. (needed for .NET 4.8)
2. Load assembly in TestHelpers.
3. It works.

internal class TestHelpers
{
    public static (ImmutableArray<Diagnostic> Diagnostics, string Output) GetGeneratedOutput<T>(string source)
        where T : IIncrementalGenerator, new()
    {
        var syntaxTree = CSharpSyntaxTree.ParseText(source);
        var references = AppDomain.CurrentDomain.GetAssemblies()
            .Where(_ => !_.IsDynamic && !string.IsNullOrWhiteSpace(_.Location))
            .Select(_ => MetadataReference.CreateFromFile(_.Location))
            .Concat(new[]
            {
                MetadataReference.CreateFromFile(typeof(T).Assembly.Location),
                MetadataReference.CreateFromFile(typeof(EnumExtensionsAttribute).Assembly.Location),
+               MetadataReference.CreateFromFile(typeof(System.ComponentModel.DataAnnotations.DisplayAttribute).Assembly.Location)
            });

        var compilation = CSharpCompilation.Create(
            "generator",
            new[] { syntaxTree },
            references,
            new CSharpCompilationOptions(OutputKind.DynamicallyLinkedLibrary));

        var originalTreeCount = compilation.SyntaxTrees.Length;
        var generator = new T();

        var driver = CSharpGeneratorDriver.Create(generator);
        driver.RunGeneratorsAndUpdateCompilation(compilation, out var outputCompilation, out var diagnostics);

        var trees = outputCompilation.SyntaxTrees.ToList();

        return (diagnostics, trees.Count != originalTreeCount ? trees[trees.Count - 1].ToString() : string.Empty);
    }
}

end of edit

If you add a reference to the correct NuGet package and load the corresponding library "it just works".

Following steps I did:

1. Add package System.ComponentModel.Annotations to NetEscapades.EnumGenerators.Tests and NetEscapades.EnumGenerators.
2. Create Instance of DisplayAttribute inside the test to load the assembly (I don't know a better way).
3. It works.

Before

.Net 4.8 without PackageReference (also notice the different attributeName)

After

.Net 4.8 with PackageReference

some code

[Fact]
public Task CanGenerateEnumExtensionsWithDisplayName()
{
    var x = new System.ComponentModel.DataAnnotations.DisplayAttribute();
    //const string input = ...
    //...
}

[adamradocz]

@FroggieFrog Thanks, made the changes.

[adamradocz]

This is good work, thanks! Before we merge it, I think we should do 2 things:

1. Add an integration test for the behaviour
2. Figure out why it doesn't work in .NET Framework

Definitely happy to merge it once we have those, thanks again!

Done.

[andrewlock]

Thanks @adamradocz, this is looking really good! 😃 Just a couple of nits, but I'm wondering if we should include the [Display] name in the TryParse() etc methods too 🤔 I'm undecided, what do you think? Also wondering if we should make this attribute functionality optional, but we could always add that later if needs be!

[adamradocz]

• Now the Display attribute is parsed back.
• Refactored the TryParse a little bit, so only the private method contains the logic.
• Added a few more tests.

[adamradocz]

BenchmarkDotNet=v0.13.1, OS=Windows 10.0.22000
11th Gen Intel Core i7-11800H 2.30GHz, 1 CPU, 16 logical and 8 physical cores
  [Host]     : .NET Framework 4.8 (4.8.4470.0), X64 RyuJIT  [AttachedDebugger]
  DefaultJob : .NET Framework 4.8 (4.8.4470.0), X64 RyuJIT

Method	Mean	Error	StdDev	Ratio	RatioSD	Gen 0	Allocated
EnumIsDefined	35.902 ns	0.6696 ns	0.6263 ns	1.00	0.00	-	-
ExtensionsIsDefined	3.668 ns	0.0106 ns	0.0099 ns	0.10	0.00	-	-
EnumIsDefinedNameDisplayName	4,209.063 ns	81.6949 ns	124.7567 ns	119.50	3.04	0.1602	1,100 B
ExtensionsIsDefinedNameDisplayName	2.615 ns	0.0111 ns	0.0093 ns	0.07	0.00	-	-

BenchmarkDotNet=v0.13.1, OS=Windows 10.0.22000
11th Gen Intel Core i7-11800H 2.30GHz, 1 CPU, 16 logical and 8 physical cores
  [Host]     : .NET Framework 4.8 (4.8.4470.0), X64 RyuJIT  [AttachedDebugger]
  DefaultJob : .NET Framework 4.8 (4.8.4470.0), X64 RyuJIT

Method	Mean	Error	StdDev	Ratio	RatioSD	Gen 0	Allocated
EnumToString	231.936 ns	4.6595 ns	8.4021 ns	1.000	0.00	0.0112	72 B
EnumToStringDisplayName	2,457.371 ns	47.3430 ns	44.2846 ns	10.883	0.48	0.1259	835 B
ToStringFast	1.464 ns	0.0549 ns	0.0513 ns	0.006	0.00	-	-

BenchmarkDotNet=v0.13.1, OS=Windows 10.0.22000
11th Gen Intel Core i7-11800H 2.30GHz, 1 CPU, 16 logical and 8 physical cores
  [Host]     : .NET Framework 4.8 (4.8.4470.0), X64 RyuJIT  [AttachedDebugger]
  DefaultJob : .NET Framework 4.8 (4.8.4470.0), X64 RyuJIT

Method	Mean	Error	StdDev	Ratio	RatioSD	Gen 0	Allocated
EnumTryParse	165.700 ns	3.3072 ns	5.1489 ns	1.00	0.00	0.0200	128 B
ExtensionsTryParse	5.273 ns	0.1244 ns	0.1480 ns	0.03	0.00	-	-
EnumTryParseDisplayName	4,053.959 ns	80.5383 ns	75.3355 ns	24.99	1.12	0.1602	1,100 B
ExtensionsTryParseDisplayName	3.118 ns	0.0781 ns	0.0692 ns	0.02	0.00	-	-

[andrewlock]

I think this is looking really good @adamradocz, thanks 🙂 Just a couple of final questions/suggestions!

[adamradocz]

The optimized TryParse is a little bit faster.

Method	Mean	Error	StdDev	Ratio	Gen 0	Allocated
ExtensionsTryParse	7.615 ns	0.0208 ns	0.0194 ns	0.008	-	-
New ExtensionsTryParse	7.097 ns	0.0226 ns	0.0212 ns	0.007	-	-

[adamradocz]

@andrewlock I think, the IsDefined should have the allowMatchingDisplayAttribute parameter in its signature, even though the display attribute is not defined in the enum, just to be consistent. What do you think?

[andrewlock]

@andrewlock I think, the IsDefined should have the allowMatchingDisplayAttribute parameter in its signature, even though the display attribute is not defined in the enum, just to be consistent. What do you think?

I agree. It would be annoying (for example) if I had a [Display] attribute, originally, used the allowMatchingDisplayAttribute version, removed the attribute later, and then my code wouldn't compile.

Just a thought - I think we should support the [Description] attribute too. I'll do that in a separate PR, but I wonder if we should rename allowMatchingDisplayAttribute to allowMatchingMetadataAttribute (or something) to cover both cases. I don't really like the name as much, but maybe it's better 🤔

[adamradocz]

Just a thought - I think we should support the [Description] attribute too. I'll do that in a separate PR, but I wonder if we should rename allowMatchingDisplayAttribute to allowMatchingMetadataAttribute (or something) to cover both cases. I don't really like the name as much, but maybe it's better 🤔

How would you differentiate the two attributes then, when parsing? What if I set the [Description] and [Display] attributes as well, but I don't want the parser to match the description, only for the display?

Edit:
What if we use flags for that? That could cover all the possibilities.

[adamradocz]

IsDefined is updated.

The only questioning thing is the allowMatchingDisplayAttribute vs allowMatchingMetadataAttribute.

[FroggieFrog]

How about adding an additional property to the attribute and respect the defined order?
Something like this:

public class EnumExtensionsAttribute : System.Attribute
{
    //...
    public Type[]? AdditionalMetaData { get; set; }
}

[EnumExtensions(AdditionalMetaData = new[] { typeof(DisplayAttribute), typeof(DescriptionAttribute) })]
public enum MyEnum
{
        [Display(Name ="1st")]
        First = 0,
        [Description("2nd")]
        Second = 1,
        [Description("3rd")]
        [Display(Name = "the third")]
        Third = 2,
}

So MyEnum.Third.ToStringFast() will result in "the third", because DisplayAttribute is defined first in AdditionalMetaData.
Another name could be SupportedAttributes.

[andrewlock]

I'm undecided. Personally, I don't think it's worth fighting with this edge case too much. I would probably just use whichever is defined first. I can't imagine anyone is adding both the [Description] attribute and the [Display] attribute to their enums! If someone is, and we want to support that scenario, we could add the option later. How does that sound @FroggieFrog @adamradocz?

[adamradocz]

It's okay with me. I'll update the naming soon.

[adamradocz]

I renamed the variable. I think this PR is complete.

[andrewlock]

Sorry for the delay on merging this @adamradocz, and thanks for all your hard work! It's much appreciated 🙂