馃悰 Ensure that Optional[list] values work correctly with callbacks - #1018
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
solesensei
changed the title
|
@tiangolo, Hi! Please check this review. It blocks our application from upgrading to |
|
cc: @svlandeg |
solesensei
changed the title
solesensei
changed the title
|
Hi @solesensei, thanks for the PR! We've got this on our internal queue and will let you know once we've been able to review it 馃檹 |
svlandeg
changed the title
Optional[list] values work correctly with callbacks
|
@svlandeg I'm sorry, but could you please take a look at this PR? It's a simple one-line fix that shouldn't take much time. |
|
@solesensei: like I said, we have this on our queue and will look at it soon. Please refrain from pinging maintainers directly, as this makes our Github mentions very difficult to use and we don't just give priority to whoever pings the most 馃槈 |
|
馃摑 Docs preview for commit 07350b5 at: https://543f0da8.typertiangolo.pages.dev |
There was a problem hiding this comment.
Thanks for the PR! I've now had time to go through it in detail.
The tests you've added clearly showcase the issue on master when a callback is combined with a list convertor - giving an error when value is None at L655 of main.py.
In the test files, both test_tutorial005.py and test_tutorial005_an.py only contain one failing test, and both represent the same behaviour. Generally, tutorial files should also be included in the Typer documentation. In this case however, I don't think that adding more explanation to the documentation adds much. As such, I suggest to drastically reduce the unit test files and only test for the edge case mentioned above.
I'll go ahead and make those changes accordingly, to get the PR in a good shape to merge.
| ) -> Callable[[Sequence[Any]], Optional[List[Any]]]: | ||
| def internal_convertor(value: Sequence[Any]) -> Optional[List[Any]]: | ||
| if default_value is None and len(value) == 0: | ||
| return None | ||
| ) -> Callable[[Optional[Sequence[Any]]], Optional[List[Any]]]: | ||
| def internal_convertor(value: Optional[Sequence[Any]]) -> Optional[List[Any]]: | ||
| if value is None or len(value) == 0: | ||
| return default_value |
There was a problem hiding this comment.
I've changed the check to be more extensive and return default_value any time that value is None or empty. This needed to be updated when fixing the type, or the linter would complain about value possible being None when running the list comprehension.
svlandeg
left a comment
svlandeg
left a comment
There was a problem hiding this comment.
This should now be ready for a final review by Tiangolo!
svlandeg
added
bug
|
Hey! 馃憢 Any updates on this? |
|
Hi, @tiangolo! Do you have a moment to look at this PR? |
3 participants
This pull request refactors the
generate_list_convertorfunction to handle the case when thevalueparameter isNone.Fixes: #762