Add support for Collection[X] | Y (for some collection types)#745
Add support for Collection[X] | Y (for some collection types)#745jhominal wants to merge 1 commit intopython-attrs:mainfrom
Conversation
|
Just letting you know this is on my radar, I'm feeling under the weather this week so it might have to wait a little |
|
Thank you for commenting about your status! Do not worry about it - there is really nothing on the line here except a possible improvement to users of |
Tinche
force-pushed
the
dispatch-single-collection
branch
from
e8add33 to
feb2f5b
Compare
Tinche
left a comment
Tinche
left a comment
There was a problem hiding this comment.
Looks very solid to me. Please proceed with the changelog entry and some documentation.
| ) -> Any: | ||
| # Design choice: only detect known concrete types as valid source types | ||
| # That avoids having to blacklist e.g. str or bytes | ||
| if isinstance(val, (deque, frozenset, list, set, tuple)): |
There was a problem hiding this comment.
I was going to ask if we can test against the exact type we're expecting, but that might be too restrictive. It's very common to structure lists into tuples/frozensets etc. So I think this is perfectly fine.
| # Design choice: only detect known concrete types as valid source types | ||
| # That avoids having to blacklist e.g. str or bytes | ||
| if isinstance(val, (deque, frozenset, list, set, tuple)): | ||
| return converter.structure(val, collection_type) |
There was a problem hiding this comment.
A trick you can do is finding these hooks in the hook factory itself (so in make_structure_union_single_collection, instead of in structure_union_single_collection) using converter.get_structure_hook, and then just using them here.
2 participants
fixes #743
As discussed in the issue, here is the PR.
Compared to my last comment on the issue:
deque,setandfrozenset. In case you would prefer not supporting these types, I can remove them (I am not sure whether the added support is "worth it" or not.)I must admit, I was not too inspired for tests, there are probably more tests that could be added. The PR is also missing documentation.