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

feat: add StringEnum<TEnum> #98

Merged
merged 2 commits into from
Jun 8, 2023
Merged

feat: add StringEnum<TEnum> #98

merged 2 commits into from
Jun 8, 2023

Conversation

JamieMagee
Copy link
Contributor

@JamieMagee JamieMagee commented Jul 26, 2022

This PR adds the StringEnum<TEnum>, similar to Octokit's version1. See Octokit documentation for more explanation and reasoning behind this approach2

TODO:

  • Handle IEnumerable<StringEnum<TEnum>> with a custom JsonConvertorFactory3
  • Refactor ToEnumString and ToEnum helper methods to extension methods (or somewhere else)
  • StringEnumConverter (or StringEnumConverterFactory) tests

Closes #90

Footnotes

  1. https://github.com/octokit/octokit.net/blob/5fe3ea82f03b7fe64bcf8145f3b127fa2c585665/Octokit/Models/Response/StringEnum.cs#L11

  2. https://github.com/octokit/octokit.net/blob/main/docs/working-with-enums.md

  3. https://docs.microsoft.com/en-us/dotnet/standard/serialization/system-text-json-converters-how-to?pivots=dotnet-6-0#sample-factory-pattern-converter

@JamieMagee JamieMagee added Type: Breaking change Used to note any change that requires a major version bump version:major labels Jul 26, 2022
@JamieMagee JamieMagee force-pushed the feat/string-enum branch 2 times, most recently from f1f4604 to 46cb083 Compare August 9, 2022 04:33
@JamieMagee JamieMagee force-pushed the feat/string-enum branch 2 times, most recently from 4b33c6e to 24cc656 Compare December 6, 2022 05:50
@JamieMagee JamieMagee marked this pull request as ready for review December 6, 2022 05:58
@JamieMagee
Copy link
Contributor Author

@martincostello I think this is ready for review

martincostello
martincostello previously approved these changes Dec 7, 2022
@JamieMagee
Copy link
Contributor Author

Thanks for the review @martincostello! There are a couple of model updates that I'd like to merge and get into a patch release, before merging this and cutting a major release.

@JamieMagee JamieMagee enabled auto-merge (squash) June 7, 2023 16:57
@JamieMagee JamieMagee disabled auto-merge June 7, 2023 16:57
@JamieMagee
Copy link
Contributor Author

I pushed an update, but it looks like the PR isn't receiving updates 😢

@JamieMagee JamieMagee force-pushed the feat/string-enum branch 2 times, most recently from 5a326ef to f9f5885 Compare June 7, 2023 17:58
Copy link
Member

@kfcampbell kfcampbell left a comment

Choose a reason for hiding this comment

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

Thanks for all your work here!

@JamieMagee JamieMagee merged commit ba8a3da into main Jun 8, 2023
@JamieMagee JamieMagee deleted the feat/string-enum branch June 8, 2023 22:52
@JamieMagee
Copy link
Contributor Author

@martincostello I just cut the v2.0.0 release. Can you upgrade and let me know how it goes?

@martincostello
Copy link
Contributor

Sure - I'll poke dependabot on my app and see what happens 😄

@martincostello
Copy link
Contributor

I just had to make a few minor adjustments to fix the compilation and maintain the behaviour: martincostello/costellobot@3e552ca

@JamieMagee
Copy link
Contributor Author

Those changes look like what I would expect.

Thanks for all your help with this change!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Breaking change Used to note any change that requires a major version bump
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider not throwing exceptions when deserializing unknown enum values
4 participants