Skip to content

Conversation

akarboush
Copy link
Contributor

@akarboush akarboush commented Feb 21, 2024

Fixes #494
Fixes #521

@fiseni
Copy link

fiseni commented Apr 26, 2024

We don't need to expose a new API here. Instead, we need to fix the SmartEnum.List. Right now, it reads from the _fromName dictionary and creates a brand new list for the output.
That's unnecessary, we already hold an _enumOptions field which is the list of all enums. So, we need only the following fix

public static IReadOnlyCollection<TEnum> List => _enumOptions.Value;

@akarboush
Copy link
Contributor Author

@fiseni, you are actually correct! I completely overlooked the _enumOptions option.
I'll go ahead with it if that's alright with you, @ardalis.

@fiseni
Copy link

fiseni commented Apr 27, 2024

@akarboush @ardalis
There is more to it. I created issue #521 describing the details and the proposed changes.

Copy link

@fiseni fiseni left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the related issue, I suggested returning IReadOnlyList so the users may use also for iteration. Since it inherits from IReadOnlyCollection it won't be a breaking change.

Also, I suggested keeping the state as ReadOnlyCollection, otherwise the users will be able to cast back to an array.

Anyhow, @ardalis let's merge this PR as is. We'll tackle those details additionally.

@ardalis ardalis merged commit 7396d73 into ardalis:main Apr 28, 2024
@ardalis
Copy link
Owner

ardalis commented Apr 28, 2024

Done

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 this pull request may close these issues.

Avoid creating a new list on SmartEnum.List Add a way to enumerate over the List to prevent allocation every time List is called
3 participants