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 union #42

Merged
merged 9 commits into from Mar 7, 2023
Merged

Support union #42

merged 9 commits into from Mar 7, 2023

Conversation

SvenWarnke
Copy link
Contributor

This addresses #41 . It allows Union types again by simply removing the raising of the exception. I also handled 2 test that showcase the behavior.

@mivade
Copy link
Owner

mivade commented Mar 2, 2023

I can see there being utility in union types (although I personally would rarely use that feature), but I'm not sure if just passing through the value like this is the right way to go. Maybe if it's not an optional type we should require a type given in the metadata? Either that or some special flag to just skip the logic of determining the type entirely.

argparse_dataclass.py Outdated Show resolved Hide resolved
@jcal-15
Copy link
Contributor

jcal-15 commented Mar 2, 2023

I can see there being utility in union types (although I personally would rarely use that feature), but I'm not sure if just passing through the value like this is the right way to go. Maybe if it's not an optional type we should require a type given in the metadata? Either that or some special flag to just skip the logic of determining the type entirely.

I see I just said what you said, but less articulately. 😄

@SvenWarnke SvenWarnke requested review from jcal-15 and mivade and removed request for jcal-15 March 3, 2023 07:32
@mivade
Copy link
Owner

mivade commented Mar 3, 2023

It looks like CI is failing because of Black checks. Once tests are passing this looks good to merge.

We also need to update the documentation to explain how to use union types with some examples. I am happy to merge this as is though and create a new issue to track that.

@SvenWarnke
Copy link
Contributor Author

I ran Black on the code, I hope it will pass now.

@mivade
Copy link
Owner

mivade commented Mar 7, 2023

Thanks!

@mivade mivade merged commit 5919cf2 into mivade:main Mar 7, 2023
@SvenWarnke SvenWarnke deleted the Support-Union branch March 9, 2023 15:02
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.

None yet

3 participants