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

🐛 Fix default value of None for CLI Parameters when the type is list | None and the default value is None #664

Merged
merged 5 commits into from Mar 23, 2024

Conversation

theowisear
Copy link
Contributor

If an optional list type option has default_value set to None, we expect its value to also be set to None if no option has been given to the command.

Related to issue #410

@theowisear theowisear changed the title List type options return None Optional[List] type options return None Sep 13, 2023
Copy link
Collaborator

@svlandeg svlandeg left a comment

Choose a reason for hiding this comment

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

Thanks!

This PR still requires a test to check for the correct behaviour - that also makes it easier to review as there's a minimal example to test on.

@svlandeg svlandeg marked this pull request as draft February 29, 2024 16:34
@svlandeg svlandeg added feature New feature, enhancement or request p3 labels Feb 29, 2024
@svlandeg svlandeg added bug Something isn't working p2 and removed feature New feature, enhancement or request p3 labels Mar 22, 2024
@svlandeg svlandeg linked an issue Mar 22, 2024 that may be closed by this pull request
Copy link

📝 Docs preview for commit 8d6aaf0 at: https://044fd36b.typertiangolo.pages.dev

Copy link
Collaborator

@svlandeg svlandeg left a comment

Choose a reason for hiding this comment

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

I've added an example in the docs and a unit test, and confirmed that this PR indeed changes the type of the argument to None as you would expect.

The fix itself looks good to me, but I'll leave this one up for review by @tiangolo to get his opinion as well.

@svlandeg svlandeg marked this pull request as ready for review March 22, 2024 15:35
Copy link

📝 Docs preview for commit beaf3f1 at: https://331f9f9c.typertiangolo.pages.dev

@tiangolo tiangolo changed the title Optional[List] type options return None 🐛 Fix default value of None for CLI Parameters when the type is list | None and the default value is None Mar 23, 2024
@tiangolo tiangolo merged commit cf3290d into tiangolo:master Mar 23, 2024
18 checks passed
@tiangolo
Copy link
Owner

Great, thank you @theowisear! 🍪

And thanks for the help here @svlandeg! 🙇 🍰

This will be available in Typer 0.10.0, in the next few hours/minutes. 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working p2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Optional multi-value option returns empty tuple instead of None
3 participants