Codecov Report

Merging #219 (312cc2c) into main (989d2d7) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #219      +/-   ##
==========================================
- Coverage   98.34%   98.32%   -0.02%     
==========================================
  Files          16       16              
  Lines         723      718       -5     
==========================================
- Hits          711      706       -5     
  Misses         12       12              

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 989d2d7...312cc2c. Read the comment docs.

Is this ready for review?

Hi @Tinche, made some dumb mistakes prior, but now, it is ready! :)

The implementation looks good, it needs a changelog entry and possibly documentation updates.

We already have a lot of Hypothesis tests though, I don't want to add new ones. What we should do is change the existing tests to test for these. I will push a branch with some modified tests you can rebase on and see what I mean.

OK, I can make a changelog entry.

Regarding the hypothesis tests, would it make sense to hide my added tests behind conditional skip - is_py38 or is_py37?

Here's the new branch: https://github.com/python-attrs/cattrs/tree/tin/new-tests

If you look at the last commit, I added bare list, dict, set and frozenset to the strategies and enabled them for 3.7/3.8. So the strategies are already parametrized ;)

And also, that is for a separate issue, but docs for contributing are outdated - poetry does not support editable installs -

$ pip install -e.
Obtaining file:///home/libor/Desktop/Programovani/Pythony/forked/cattrs
  Installing build dependencies ... done
  Checking if build backend supports build_editable ... done
ERROR: Project file:///home/libor/Desktop/Programovani/Pythony/forked/cattrs has a 'pyproject.toml' and its build backend is missing the 'build_editable' hook. Since it does not have a 'setup.py' nor a 'setup.cfg', it cannot be installed in editable mode. Consider using a build backend that supports PEP 660.

Ah, we were using the alpha release of Poetry 1.2 for a while, which does support this, but fell back to 1.1 for other reasons.

Ok, checked the branch you posted, I will

1. revert all the changes in tests/ directory on this branch
2. cherry-pick your commit
3. add changelog entry
4. push and profit

Sounds good. I think I missed a few, like tuple?

Ah, true, so will in the types you missed and $$$

Hey @Tinche, it's ready for review -

I have added changelog entry, tests for tuples and also did some clean-up of collections.abc. aliases that were there for some reason, but I do not understand why exactly, when you can import them from typing.

Did I miss something?

Well, it seems that tests uncovered another failing case:

>>> import attrs
>>> import cattrs
>>> from typing import Tuple

>>> @attrs.define
... class A:
...     a: Tuple = attrs.field(default=('', '', ''))

>>> cattrs.unstructure(x)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/libor/.virtualenvs/p38/lib/python3.8/site-packages/cattr/converters.py", line 197, in unstructure
    return self._unstructure_func.dispatch(
  File "/home/libor/.virtualenvs/p38/lib/python3.8/site-packages/cattr/dispatch.py", line 49, in _dispatch
    return self._function_dispatch.dispatch(cl)
  File "/home/libor/.virtualenvs/p38/lib/python3.8/site-packages/cattr/dispatch.py", line 126, in dispatch
    return handler(typ)
  File "/home/libor/.virtualenvs/p38/lib/python3.8/site-packages/cattr/converters.py", line 722, in gen_unstructure_attrs_fromdict
    h = make_dict_unstructure_fn(
  File "/home/libor/.virtualenvs/p38/lib/python3.8/site-packages/cattr/gen.py", line 125, in make_dict_unstructure_fn
    handler = converter._unstructure_func.dispatch(t)
  File "/home/libor/.virtualenvs/p38/lib/python3.8/site-packages/cattr/dispatch.py", line 49, in _dispatch
    return self._function_dispatch.dispatch(cl)
  File "/home/libor/.virtualenvs/p38/lib/python3.8/site-packages/cattr/dispatch.py", line 126, in dispatch
    return handler(typ)
  File "/home/libor/.virtualenvs/p38/lib/python3.8/site-packages/cattr/converters.py", line 754, in gen_unstructure_iterable
    h = make_iterable_unstructure_fn(
  File "/home/libor/.virtualenvs/p38/lib/python3.8/site-packages/cattr/gen.py", line 339, in make_iterable_unstructure_fn
    type_arg = get_args(cl)[0]
IndexError: tuple index out of range

Again, failed on py37/8, is OK on py39 (and pressumably py310)

I have pushed 2 commits

• since I discovered that Tuple.__args__ == (), I added this check to is_bare.
• add the same check in unstructuring iterable, because tests were failing on unstructuring unannotated Tuples

What remains to be fixed are tests for roundtrips of un/struct of sets and frozen sets.

I am out for today, will investigate later.

Cheers, Libor

Also, all the checks around the code for if getattr(..., '__args__', None) ,,, seem to be very fragile and I suggest to consolidate them in one place - I understand it's a pain in the ass, since 4 versions of Python are supported and typing in Python has undergone some huge development, but it seems to be quite a source of bugs here. :-/

I'm gonna have to ask you to return the abc generics. On newer Pythons, classes like typing.MutableSequence are deprecated in favor of collections.abc.MutableSequence, so we need to test them too.

(https://docs.python.org/3/library/typing.html#typing.MutableSequence)

I'm gonna have to ask you to return the abc generics. On newer Pythons, classes like typing.MutableSequence are deprecated in favor of collections.abc.MutableSequence, so we need to test them too.

(https://docs.python.org/3/library/typing.html#typing.MutableSequence)

Yes, got it, that's what I missed from the documentation.

OK, now, it should be complete:

• I have refactored the is_bare check as a set membership
• Added a changelog mentioning the Tuple structuring
• Finished the tests/metadata/__init__.py

What do you think? @Tinche

Looks great! I think the changelog format needs to be tweaked but I can do that myself. Thanks!