-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Remove DAM annotations from enum converter #100347
Conversation
Note regarding the
|
Tagging subscribers to this area: @dotnet/area-system-componentmodel |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM but I'd appreciate if @MichalStrehovsky could take a look too.
src/libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/EnumConverter.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/EnumConverter.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.ComponentModel.TypeConverter/src/Resources/Strings.resx
Outdated
Show resolved
Hide resolved
src/libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/EnumConverter.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/EnumConverter.cs
Outdated
Show resolved
Hide resolved
...tem.ComponentModel.TypeConverter/src/System/ComponentModel/ReflectTypeDescriptionProvider.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/EnumConverter.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/EnumConverter.cs
Show resolved
Hide resolved
src/libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/EnumConverter.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good from trimming perspective. Can't talk about TypeConverter or why we accept System.Enum (and whether this should accept all non-enums for the same reasons it accepts System.Enum).
{ | ||
if (!type.IsEnum && !type.Equals(typeof(Enum))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a little bit concerned we have tests where this is used with a non-enum and it apparently works (System.Enum is really no different from just System.Object, or whatever).
* Remove DAM annotations from enum converter * expand the debug assert for enum types * FB * FB2
Seems there are a few others who might be broken by this: https://github.com/search?q=%2Fbase%5C%28type%5C.GetType%5C%28%5C%29%5C%29%2F+EnumConverter&type=code |
Remove DAM annotations from Enum converter since the trimmer always preserve
enums
.