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

Add unstructuring and structuring support for deque in standard lib #355

Merged
merged 5 commits into from
May 22, 2023

Conversation

lqhuang
Copy link
Contributor

@lqhuang lqhuang commented Apr 19, 2023

Env

Python==3.10.4
attrs==22.2.0
cattrs==23.1.0.dev0@98ca33ab7

What's the problem

Current head version of cattrs work normally to unstructure deque, but fails to structure obj into deque (or generic deque)

Minimal reproducible example:

@define
class C:
    x: int
    y: int

# OK
a = deque((C(x=x, y=y) for x, y in zip(range(10), range(10, 20))))
print(cattrs.unstructure(a))

# Bad
b = ({"x": x, "y": y} for x, y in zip(range(10), range(10, 20)))
print(cattrs.structure(b, deque[C]))
# Traceback (most recent call last):
#   File ".../cattrs/mycase.py", line 33, in <module>
#     main()
#   File ".../cattrs/mycase.py", line 29, in main
#     print(cattrs.structure(a, deque[C]))
#   File ".../cattrs/src/cattrs/converters.py", line 330, in structure
#     return self._structure_func.dispatch(cl)(obj, cl)
#   File ".../cattrs/src/cattrs/converters.py", line 398, in _structure_error
#     raise StructureHandlerNotFoundError(msg, type_=cl)
# cattrs.errors.StructureHandlerNotFoundError: Unsupported type: collections.deque[__main__.C]. Register a structure hook for it.

deque is a battery-included std container, so I think it's sensible to add a default structure hook for it.

Some notable facts:

print(issubclass(deque, list))
# False
print(issubclass(defaultdict, dict))
# True

I once want to also add a structure hook for defaultdict, but I thought it's probably difficult to give a factory func for defaultdict, which is better to custom by user.

What this PR does

  1. Implement function _structure_deque() and is_deque()
  2. Add related unit tests (test_structuring, test_converters)
  3. Update docs and history.

Suggestions and improvements are appreciated!

Thanks :)

@Tinche
Copy link
Member

Tinche commented Apr 19, 2023

Thanks! I'm going to PyCon soon so my attention here will be somewhat delayed, hope that's ok.

@lqhuang
Copy link
Contributor Author

lqhuang commented Apr 20, 2023

Never mind, enjoy your trip! Have fun 🤣

Looks like I need to fix some compatibility issues. I only test 3.10 on local yet.

@lqhuang
Copy link
Contributor Author

lqhuang commented Apr 20, 2023

@Tinche Updated. Could you help to let CI run again?

@codecov-commenter
Copy link

codecov-commenter commented Apr 20, 2023

Codecov Report

Merging #355 (baee96b) into main (27e9c0d) will decrease coverage by 8.19%.
The diff coverage is 70.00%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff             @@
##             main     #355      +/-   ##
==========================================
- Coverage   97.55%   89.36%   -8.19%     
==========================================
  Files          21       21              
  Lines        1675     1702      +27     
==========================================
- Hits         1634     1521     -113     
- Misses         41      181     +140     
Impacted Files Coverage Δ
src/cattrs/_compat.py 59.89% <66.66%> (-37.44%) ⬇️
src/cattrs/converters.py 92.19% <70.83%> (-5.76%) ⬇️

... and 5 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@lqhuang
Copy link
Contributor Author

lqhuang commented Apr 20, 2023

@Tinche PR is ready to review. But follow your schedules, it's fine.

@Tinche Tinche self-requested a review May 18, 2023 22:54
@lqhuang lqhuang requested a review from Tinche May 19, 2023 03:12
@Tinche Tinche added this to the 23.1 milestone May 21, 2023
Copy link
Member

@Tinche Tinche left a comment

Choose a reason for hiding this comment

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

Left a comment, other than that, looks good!

@lqhuang
Copy link
Contributor Author

lqhuang commented May 22, 2023

Oh. You're right! 😅 My bad

Run a real doctest

Python 3.10.11 (main, Apr  7 2023, 07:31:31) [Clang 14.0.0 (clang-1400.0.29.202)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import cattrs
>>> from collections import deque
>>> from typing import Optional
>>> cattrs.structure((1, None, 3), deque[Optional[str]])
deque(['1', None, '3'])
>>> cattrs.structure((1, 2, 3), deque[int])
deque([1, 2, 3])

@Tinche Tinche self-requested a review May 22, 2023 11:54
@Tinche
Copy link
Member

Tinche commented May 22, 2023

Thanks @lqhuang , good job. Please resolve the changelog merge conflict and let's get this in!

@lqhuang lqhuang force-pushed the add-deque-support branch from fe6ebf4 to baee96b Compare May 22, 2023 13:12
@lqhuang
Copy link
Contributor Author

lqhuang commented May 22, 2023

Already rebased from main. Thank you so much @Tinche !

@Tinche Tinche merged commit cec0035 into python-attrs:main May 22, 2023
@lqhuang lqhuang deleted the add-deque-support branch May 23, 2023 10:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants