Localize Enum Display names #5185

ryanbrandenburg · 2016-08-25T17:00:37Z

This PR addresses a bug and a couple missing features.

The bug (#5198) is that Enum display values would previously store the first value that was used and always use that. By passing a function which returns a Name value to the constructor of EnumGroupAndName and executing that when we need the name we avoid this.

One missing feature is #5197, that GetEnumSelectList was checking the DisplayAttributes for values to display, but it wasn't localizing those values like other uses of DisplayAttributes do.

The other missing feature was #4215. It boils down to that Html.DisplayFor didn't pay attention to DisplayAttributes on enums, and consequently didn't localize them.

ryanbrandenburg · 2016-08-25T17:05:05Z

src/Microsoft.AspNetCore.Mvc.Abstractions/ModelBinding/EnumGroupAndName.cs

+            _nameFunc = () => name;
+        }
+
+        public EnumGroupAndName(string group, Func<string> name)


I couldn't decide if we should Obsolete the old constructor. My instinct was no, to allow people to do the simple thing if they didn't need anything complex, but I fear that will lead to ignoring the new constructor and getting us right back into the same spot.

Also, going back to add comments to this and any new public methods now.

Yeah I'm not sure if it needs to be obsoleted. If the docs just said precisely what it did and didn't do, that would seem to be fine.

dougbu · 2016-08-25T17:21:15Z

I'll take reviewing this one.

ryanbrandenburg · 2016-08-25T17:22:09Z

CC @Eilon and @dougbu. I know this isn't exactly what we had talked about (adding new Enum properties to the DisplayName context) but once I got down into it it seemed cleaner this way.

dougbu · 2016-08-25T17:31:20Z

The bug is that Enum value localization

When submitting the final commit, please update the description to avoid the implication that [Display(Name = "Fred")] is only about localization. The attribute is in general about UI overrides, in some cases including localization of the displayed strings.

dougbu · 2016-08-25T17:33:07Z

src/Microsoft.AspNetCore.Mvc.Abstractions/ModelBinding/EnumGroupAndName.cs

+        /// </summary>
+        /// <param name="group">The group name.</param>
+        /// <param name="name">A function which returns the name. (Necessary for proper localization.)</param>
+        public EnumGroupAndName(string group, Func<string> name)


Probably should [Obsolete] the old constructor and file a 2.0 bug about removing it.

dougbu · 2016-08-25T18:20:28Z

This PR addresses a bug

What bug number? Do we have anything tracking the bug or either of the gaps mentioned in the description?

Not sure about doing the three separate pieces in one PR. I'm not confident the added tests are covering each bit sufficiently.

dougbu · 2016-08-25T18:20:35Z

⌚

ryanbrandenburg · 2016-08-31T17:09:41Z

🆙📅 I started to split this up into three separate PR's but they're connected enough that it was going to take more time than I though was really prudent to spend on it. Are there any specific test case you feel are missing?

dougbu · 2016-08-31T17:45:42Z

src/Microsoft.AspNetCore.Mvc.Abstractions/ModelBinding/EnumGroupAndName.cs

        /// <summary>
-        /// Initializes a new instance of the EnumGroupAndName structure.
+        /// Initializes a new instance of the EnumGroupAndName structure. Should not be used if localization is in use.


What does "in use" mean? This method should not be used in any site where users may choose multiple locales.

dougbu · 2016-09-01T22:35:20Z

...icrosoft.AspNetCore.Mvc.DataAnnotations.Test/Internal/DataAnnotationsMetadataProviderTest.cs

+        public void CreateDisplayMetadata_EnumGroupedDisplayNamesAndValues_NameWithIStringLocalizer()
+        {
+            // Arrange & Act
+            var enumNameAndGroup = GetLocalizedEnumGroupedDisplayNamesAndValues();


enumGroupAndName

dougbu · 2016-09-01T22:48:19Z

⌚ mainly for EnumGroupAndNameTest refactoring

ryanbrandenburg · 2016-09-02T00:07:21Z

...icrosoft.AspNetCore.Mvc.DataAnnotations.Test/Internal/DataAnnotationsMetadataProviderTest.cs

@@ -728,6 +779,46 @@ public void CreateDisplayMetadata_IsFlagsEnum_ReflectsModelType(Type type, bool
        }


CreateDisplayMetadata_EnumGroupedDisplayNamesAndValues_ReflectsModelType fails with our new EnumGroupAndName.Equals because all the EnumGroupAndName's in the MemberData don't use FieldInfo (and thus _displayattribute). Would it be enough for this test to check that the Name and group values on EnumGroupedDisplayNamesAndValues are what they should be? Because changing the MemberData to use the FieldInfo's would be basically testing that it equals itself, correct?

Yeah, best to check Name and Group in that test. Might want to create a EnumGroupAndNameComparer if that needs to be done in more than one place.

🙈 this comment thread is no longer relevant.

ryanbrandenburg · 2016-09-06T17:36:59Z

🆙📅 Good idea on the comparer, made it much cleaner than the silly foreach zip I had started trying to do.

ryanbrandenburg · 2016-09-07T00:12:34Z

🆙📅

dougbu · 2016-09-07T01:47:07Z

src/Microsoft.AspNetCore.Mvc.Abstractions/ModelBinding/EnumGroupAndName.cs

+        /// Initializes a new instance of the EnumGroupAndName structure.
+        /// </summary>
+        /// <param name="group">The group name.</param>
+        /// <param name="name">A func which will return the name</param>


End w/ a period.

dougbu · 2016-09-07T02:26:24Z

⌚ for some cleanup

ryanbrandenburg · 2016-09-07T17:30:40Z

🆙📅

dougbu · 2016-09-07T18:08:37Z

test/Microsoft.AspNetCore.Mvc.DataAnnotations.Test/Internal/TestResources.cs

-        public static string DisplayAttribute_Description { get; } = Resources.DisplayAttribute_Description;
+        public static string Type_Three_Name => "type three name " + CultureInfo.CurrentCulture;
+        public static string Type_Three_Description => "type three description " + CultureInfo.CurrentCulture;
+        public static string Type_Three_Prompt => "type three prompt " + CultureInfo.CurrentCulture;


Please double-check that [ReplaceCulture] or using (new CultureReplacer(...)) is used in all tests referencing the CurrentCulture properties from this class.

Hmm, didn't notice Type_Three_Description and Type_Three_Prompt aren't referenced. Remove those two.

dougbu · 2016-09-07T18:12:40Z

⌚ for the last couple of wrinkles to be ironed out.

ryanbrandenburg · 2016-09-07T18:58:07Z

🆙📅

dougbu · 2016-09-07T19:33:05Z

w/ one removal and a better commit description. Also mention the relevant bugs.

ryanbrandenburg · 2016-09-07T21:14:51Z

@dougbu are you talking about the actual commit message or about the Pull Request description? Because I already added the # for the 3 bugs and removed (to my eyes) the implication that it's only about localization on the Pull Request description.

Fixes #5197 GetEnumSelectList uses IStringLocalizer Fixes #4215 Html.DisplayFor now checks DisplayAttributes on enums

dnfclas added the cla-already-signed label Aug 25, 2016

ryanbrandenburg reviewed Aug 25, 2016
View reviewed changes

dougbu self-assigned this Aug 25, 2016

dougbu reviewed Aug 25, 2016
View reviewed changes

ryanbrandenburg force-pushed the rybrande/DisplayNameLocalizer branch from 5b2a912 to ae50929 Compare August 30, 2016 17:32

dougbu reviewed Aug 31, 2016
View reviewed changes

dougbu reviewed Sep 1, 2016
View reviewed changes

ryanbrandenburg reviewed Sep 2, 2016
View reviewed changes

dougbu reviewed Sep 7, 2016
View reviewed changes

ryanbrandenburg force-pushed the rybrande/DisplayNameLocalizer branch from 69c912b to a02f29f Compare September 7, 2016 18:57

Fixes #5198 Stops caching of Enum display values

68bc79e

Fixes #5197 GetEnumSelectList uses IStringLocalizer Fixes #4215 Html.DisplayFor now checks DisplayAttributes on enums

ryanbrandenburg force-pushed the rybrande/DisplayNameLocalizer branch from a02f29f to 68bc79e Compare September 7, 2016 21:45

ryanbrandenburg merged commit 830983a into dev Sep 7, 2016

ryanbrandenburg deleted the rybrande/DisplayNameLocalizer branch September 7, 2016 23:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Localize Enum Display names #5185

Localize Enum Display names #5185

ryanbrandenburg commented Aug 25, 2016 •

edited

Loading

ryanbrandenburg Aug 25, 2016 •

edited

Loading

Eilon Aug 25, 2016

dougbu commented Aug 25, 2016

ryanbrandenburg commented Aug 25, 2016

dougbu commented Aug 25, 2016

dougbu Aug 25, 2016

dougbu commented Aug 25, 2016

dougbu commented Aug 25, 2016

ryanbrandenburg commented Aug 31, 2016

dougbu Aug 31, 2016

dougbu Sep 1, 2016

dougbu commented Sep 1, 2016

ryanbrandenburg Sep 2, 2016

dougbu Sep 2, 2016

dougbu Sep 7, 2016

ryanbrandenburg commented Sep 6, 2016

ryanbrandenburg commented Sep 7, 2016

dougbu Sep 7, 2016

dougbu commented Sep 7, 2016

ryanbrandenburg commented Sep 7, 2016

dougbu Sep 7, 2016

dougbu Sep 7, 2016

dougbu commented Sep 7, 2016

ryanbrandenburg commented Sep 7, 2016

dougbu commented Sep 7, 2016

ryanbrandenburg commented Sep 7, 2016

		@@ -728,6 +779,46 @@ public void CreateDisplayMetadata_IsFlagsEnum_ReflectsModelType(Type type, bool
		}

Localize Enum Display names #5185

Localize Enum Display names #5185

Conversation

ryanbrandenburg commented Aug 25, 2016 • edited Loading

ryanbrandenburg Aug 25, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dougbu commented Aug 25, 2016

ryanbrandenburg commented Aug 25, 2016

dougbu commented Aug 25, 2016

Choose a reason for hiding this comment

dougbu commented Aug 25, 2016

dougbu commented Aug 25, 2016

ryanbrandenburg commented Aug 31, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dougbu commented Sep 1, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryanbrandenburg commented Sep 6, 2016

ryanbrandenburg commented Sep 7, 2016

Choose a reason for hiding this comment

dougbu commented Sep 7, 2016

ryanbrandenburg commented Sep 7, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dougbu commented Sep 7, 2016

ryanbrandenburg commented Sep 7, 2016

dougbu commented Sep 7, 2016

ryanbrandenburg commented Sep 7, 2016

ryanbrandenburg commented Aug 25, 2016 •

edited

Loading

ryanbrandenburg Aug 25, 2016 •

edited

Loading