Skip to content
This repository was archived by the owner on Dec 14, 2018. It is now read-only.

GetEnumSelectList does not appear to respect Order property of DisplayAttribute #4297

Closed
tuespetre opened this issue Mar 15, 2016 · 8 comments
Assignees

Comments

@tuespetre
Copy link
Contributor

I would expect GetEnumSelectList<TEnum> to respect the Order property of any DisplayAttribute defined on the enum's members. For groups, I would expect the Order property to apply within the scope of a group. Using rc1 this does not appear to be the case.

Perhaps this could be done when building up an instance of DataAnnonationsMetadataProvider.

@dougbu
Copy link
Member

dougbu commented Mar 16, 2016

Might not be a bad idea though I'm not convinced [Display(Order = x)] is common on enum fields.

Note ModelMetadata.EnumGroupedDisplayNamesAndValues and ...EnumNamesAndValues already have a strict order. They match Enum.GetNames() and that, in turn, sorts enum names and values using their values. I'm therefore curious: What scenario leads to a poor <option> ordering when using the existing GetEnumSelectList<TEnum>() implementation?

@Eilon Eilon added the bug label Mar 16, 2016
@Eilon Eilon added this to the Backlog milestone Mar 16, 2016
@tuespetre
Copy link
Contributor Author

In cases where an enum's members' underlying values are implicitly assigned (something I hate to be honest) one can re-order the members for purely presentational purposes without breaking things by first going through and explicitly declaring the underlying values. But even when an enum's underlying values are explicitly declared, it makes more sense from a developer's point of view to keep the members ordered by their underlying values, especially when new members may be added over time for business reasons. But from the user's point of view, members that are added over time may need to appear in a different order than they do in code.

So it's kind of a small thing, but if we're committed to the [Display] attribute why not go all the way? 😉

@Eilon
Copy link
Member

Eilon commented Mar 17, 2016

Yeah this is all fine and I'd be ok one day implementing this, but it's just not a priority for shipping right now.

@tuespetre
Copy link
Contributor Author

tuespetre commented Mar 22, 2017

@Eilon @dougbu EnumGroupedDisplayNamesAndValues is an IEnumerable<KeyValuePair<EnumGroupAndName, string>> and underneath you've guaranteed the order by using a List<KeyValuePair<EnumGroupAndName, string>>. EnumNamesAndValues, however, is a IReadOnlyDictionary<string, string>, and dictionaries do not guarantee order.

I was planning to submit a PR for this just now but that is a breaking change... would it be considered for 2.0.0 to change that to an IEnumerable<KeyValuePair<string, string>>?

@dougbu
Copy link
Member

dougbu commented Mar 22, 2017

@tuespetre what is the use case for ordering both properties?

@tuespetre
Copy link
Contributor Author

tuespetre commented Mar 22, 2017

@dougbu good question. I guess EnumNamesAndValues is not used anywhere. for displaying the list of values.

tuespetre added a commit to tuespetre/Mvc that referenced this issue Mar 22, 2017
tuespetre added a commit to tuespetre/Mvc that referenced this issue Mar 22, 2017
@Eilon Eilon modified the milestones: 2.0.0-preview1, Backlog Mar 22, 2017
@Eilon
Copy link
Member

Eilon commented Mar 22, 2017

@dougbu can you review the PR at https://github.com/aspnet/Mvc/pull/6012/files ?

tuespetre added a commit to tuespetre/Mvc that referenced this issue Mar 23, 2017
tuespetre added a commit to tuespetre/Mvc that referenced this issue Mar 23, 2017
dougbu pushed a commit that referenced this issue Mar 27, 2017
@dougbu
Copy link
Member

dougbu commented Mar 27, 2017

575fe68

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

No branches or pull requests

3 participants