Conversation
MegaIng
left a comment
There was a problem hiding this comment.
Excluding those two minor style changes, this looks good.
Sorry, I also forgot about this usecase when reviewing the original change set.
|
|
||
|
|
||
| TextOrSlice = Union[AnyStr, 'TextSlice[AnyStr]'] | ||
| LarkInput = Union[AnyStr, TextSlice[AnyStr], Any] |
There was a problem hiding this comment.
I would write this as LarkInput = Union[TextOrSlice, Any] to make it clearer what the logic here is. Functionally they are equivalent.
There was a problem hiding this comment.
I guess Any overrides any other type in the union, but I want there to be an explicit list of the types that could be accepted in the normal case.
There was a problem hiding this comment.
IMO the explicit lists is also just above the line anyway, but sure, if you want to keep it like this that's fine.
(And yes, from a type-checking perspective having Any in there removes all checks - to fix this we would need essentially depended types which don't (yet) exists in python's typing system)
There was a problem hiding this comment.
I have a mild preference to keeping it this way.
If we really cared about typing, we could have a different function like parse_custom(). But I'm not sure if it's worth the trouble.
There was a problem hiding this comment.
It's not IMO and still a breaking change.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1562 +/- ##
==========================================
+ Coverage 89.93% 89.97% +0.03%
==========================================
Files 52 52
Lines 7987 8018 +31
==========================================
+ Hits 7183 7214 +31
Misses 804 804
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Fixes issue #1560