-
-
Notifications
You must be signed in to change notification settings - Fork 775
✨Adds the ability to use union in type signatures #1148
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
base: master
Are you sure you want to change the base?
Conversation
4990e72
to
639de2b
Compare
I am setting this to draft for now since I have no idea whether what I did is enough, I'll happily convert this to a merge request once someone will give me the OK. |
@svlandeg I don't mean to be rude, but is there any progress on getting this in? |
Hi @arielf212, thanks for your contribution adding this feature and an appropriate test! 🙏 We're currently working through a bit of a backlog of PRs, so unfortunately it may take a little while to get this properly reviewed by the team. We've put it on our internal board though, so we'll let you know here once we've been able to go through it in detail. Thanks for your patience! |
In the meantime, what would be helpful is if you can update the test suite to test all new lines of code, so that the coverage of the code base is back to 100% and the CI goes green again 🙏 |
can outside folks contribute to this pr? I'd be happy to take a stab at writing the rest of the tests if that helps |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@arielf212, thanks for working on this!
We definitely need to cover this feature with more tests..
I found a couple of issues:
par: Union[int, str, None] = None
(Type not yet supported: <class 'NoneType'>
)par: Union[int, float]
(shows[default: None] [required]
in help)
@a3ng7n, I think it would be nice if you could help with tests.
You can fork https://github.com/arielf212/typer/tree/feature/union (uncheck the "Copy the master branch only" checkbox), make changes and open PR to https://github.com/arielf212/typer/tree/feature/union repository. @arielf212 would be able to review and merge it to their branch.
Alternatively, you can write tests in your fork and send us the link to your branch. We will review and cherry-pick your commits
def opt(id_or_name: Union[int, str]): | ||
if isinstance(id_or_name, int): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests should be simpler.
@app.command()
def opt(id_or_name: Union[float, int, str]):
print(f"{id_or_name} ({type(id_or_name)})")
Something like this
main_type = types[0] | ||
origin = get_origin(main_type) | ||
if len(types) == 1: | ||
(main_type,) = types |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO, main_type = types[0]
is clearer.
Also, comment above should be updated (Handle SomeType | None and Optional[SomeType]
)
[item.value for item in annotation], | ||
case_sensitive=parameter_info.case_sensitive, | ||
) | ||
elif get_origin(annotation) is not None and is_union(get_origin(annotation)): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
elif get_origin(annotation) is not None and is_union(get_origin(annotation)): | |
elif is_union(get_origin(annotation)): |
Seems that this should work
This PR adds the ability to use real unions (a la
Union[int, str]
), as requested in #461.I am new to contributing to this Repository, and I'll happily add anything I might have missed (docs/tests/incorrect code location/etc).