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

Support enum properties in CustomClassDefinition.FromClass #61

Merged
merged 2 commits into from
Nov 21, 2024

Conversation

Metraberryy
Copy link

The documentation suggests that CustomClassDefinition.FromClass supports enum properties, but in practice it doesn't; this PR fixes that

Copy link
Owner

@dcronqvist dcronqvist left a comment

Choose a reason for hiding this comment

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

Good work on finding this! And even better that you took it upon yourself to create a PR to fix it!!

I agree with your solution, but have made a couple of comments that I'd like for you to look at before I'm happy to merge.

One thing we perhaps need to make sure is also to update the docs to say something about nested enum properties in classes:

They need to be added to the list of available custom types to the loader even if they exist in a class which we are doing FromClass on. So doing a FromEnum and then supplying it to the loader.

@Metraberryy
Copy link
Author

I'm unsure of where/what exactly to write in the documentation, but otherwise implemented your suggestions

Copy link
Owner

@dcronqvist dcronqvist left a comment

Choose a reason for hiding this comment

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

Nice work! I can create a separate issue for missing documentation on this behaviour and add it myself when I have time. No worries :)

@dcronqvist dcronqvist merged commit 416cba6 into dcronqvist:dev Nov 21, 2024
1 check passed
@Metraberryy Metraberryy deleted the enum-property-fromclass branch November 21, 2024 17:08
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.

2 participants