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.

Sign up for GitHub

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

Jump to bottom

Add Enum schema strategy #57

Closed
wants to merge 1 commit into from
Closed

Add Enum schema strategy #57

wants to merge 1 commit into from

Conversation

mbercx
Copy link

@mbercx mbercx commented Apr 18, 2021 •
edited
Loading

Fixes #46

Add a new subclass of SchemaStrategy for supporting the 'enum`
keyword.

The Enum class keeps track of a set that contains all of the values
that were provided by the 'enum' keywords in the schemas. When the
builder is converted to a schema, this set is converted to a list.

@mbercx
Add Enum schema strategy
ac6a8b9 
Add a new subclass of `SchemaStrategy` for supporting the 'enum`
keyword.

The `Enum` class keeps track of a `set` that contains all of the values
that were provided by the 'enum' keywords in the schemas. When the
builder is converted to a schema, this set is converted to a list.
@mbercx
Copy link
Author

mbercx commented Apr 18, 2021

@wolverdude I just set match_object to return False for now since I'm not sure how to implement this. Do we restrict the possible values to only those obtained from the object added? You mentioned using a seed schema in #46, but can I check if the existing schema already has an 'enum' defined?

@mbercx mbercx mentioned this pull request Apr 18, 2021
Draft
@wolverdude
Copy link
Owner

Hey, thanks for doing this! I'm not sure a separate Strategy class is warranted here though. Strategies are generally based on type, but enum is a generic keyword, so it could apply to any type. Perhaps a better strategy would be to apply this logic to the base strategy, but we also have to deal with the edge-case where there are enums from several different strategies that need to be combined, so this would require some updates to the Node class as well.

Given the complexity, it may be best if I implement this myself, but I do think support for enum is a useful feature that fits the constraints and purpose of the library.

Unrelatedly, the test failures are not your fault, but I just merged a fix for them, so just merge in master, and they should go green. Please add tests that cover all use cases related to any new features you add.

@wolverdude
Copy link
Owner

This PR has been inactive for months. @mbercx do you intend to merge this or should I close it?

@mbercx
Copy link
Author

mbercx commented Jan 21, 2022

Hi @wolverdude, sorry for the slow response! I won't have time to adapt the PR to your suggestion of adding the logic to the base class, especially since it's been a while since I looked at this. 😅 So feel free to close the PR if you think that is the better approach!

@wolverdude
Copy link
Owner

Thanks. I'll wait for upvotes or implement Enum if/when I get around to it.

@wolverdude wolverdude closed this May 12, 2022
@wolverdude wolverdude mentioned this pull request May 12, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Enhancement: Enum Support
2 participants
@mbercx @wolverdude