Skip to content
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

Introduce a way to enumerate the enum values without memory allocation #497

Merged
merged 4 commits into from
Apr 28, 2024

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
1 check passed
@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
3 participants