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

Enum order support #67

Merged
merged 5 commits into from
Dec 17, 2023
Merged

Conversation

jerone
Copy link

@jerone jerone commented Dec 7, 2023

For a project I'm working on, we needed enum order support.

This PR includes tests for enums with constant values and without values (default values).
Because I couldn't find a way to remove members from the enum, I used WithMembers to replace all members at once.
This forces the correct order and thus keeping the correct default value in sync.

The code fixer will now also append the constant value for enums.
I've included logic to detect if the value was implicit or explicit set.
Keeping it backwards-compatible.

I couldn't test the reporting part, but I extended the message for enums with the value.

Fixes #59 (comment)

@cezarypiatek
Copy link
Owner

Wow, first of all, many thanks for your contribution.

However, this new behavior is in some way a breaking change - all users will start getting errors in places where they do not expect it. It's a perfectly valid use case to have 2 enum types with the same field set but with different values. To avoid unnecessary surprises I would recommend introducing a feature flag that will enable this extra check. Please examine https://github.com/cezarypiatek/CSharpExtensions/blob/master/src/CSharpExtensions.Analyzers/ReturnValueUnusedAnalyzer.cs as an example of a configurable analyzer.

@jerone
Copy link
Author

jerone commented Dec 12, 2023

@cezarypiatek I've implemented the feature flag. Can you review the code to see if this is what you had in mind.

I was unable to use the feature flag json to build correct unit tests, due to cezarypiatek/RoslynTestKit#24
Maybe you can have a look at that too, or guide me in a better direction.

How do you test the analyzer and codefixer with real code? E.g. outside of the unit tests...

@cezarypiatek
Copy link
Owner

@jerone there are 2 more remarks, I forgot to click "submit review" yesterday 😅

@cezarypiatek cezarypiatek merged commit f88250d into cezarypiatek:master Dec 17, 2023
1 check passed
@cezarypiatek
Copy link
Owner

Thanks for the collaboration. I merged your changes, the new version should be automatically released any time soon.

@jerone jerone deleted the feature/enum-order branch December 18, 2023 07:47
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.

[Proposal] TwinType support for enums
2 participants