Structure using the value of attr.ib converter, if defined

[natemcmaster]

Resolves https://github.com/Tinche/cattrs/issues/138

If an attrs class has defined a converter with attr.ib, fallback that instead if dispatching to cattrs converters fails to find a handler.

Also, add a new option prefer_attrib_converters to use them if defined instead of handlers registered in cattrs.

[codecov]

Codecov Report

Merging #139 (d2ba3d7) into master (e772725) will decrease coverage by 0.02%.
The diff coverage is 96.07%.

❗ Current head d2ba3d7 differs from pull request most recent head 82d4ffd. Consider uploading reports for the commit 82d4ffd to get more accurate results

@@            Coverage Diff             @@
##           master     #139      +/-   ##
==========================================
- Coverage   97.82%   97.79%   -0.03%     
==========================================
  Files          14       15       +1     
  Lines         873      909      +36     
==========================================
+ Hits          854      889      +35     
- Misses         19       20       +1     

Impacted Files	Coverage Δ
src/cattr/gen.py	95.84% <91.66%> (-0.07%)	⬇️
src/cattr/converters.py	98.98% <100.00%> (+0.03%)	⬆️
src/cattr/dispatch.py	100.00% <100.00%> (ø)
src/cattr/errors.py	100.00% <100.00%> (ø)

---

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 e772725...82d4ffd. Read the comment docs.

[Tinche]

Cool, thanks for working on this.

Don't forget to add a changelog entry, and keep it updated.

This will need to be a flag though, and it needs to be disabled by default due to a) backwards compatibility, and b) it breaks a very common use case:

from typing import Sequence

import attr

from cattr import GenConverter

c = GenConverter()


@attr.define
class Inner:
    a: int


@attr.define
class Outer:
    inners: Sequence[Inner] = attr.field(converter=tuple)


print(c.structure({"inners": [{"a": 1}]}, Outer))

So this flag will need documentation too.

I wonder if a good heuristic would be: while generating the structure function for a class, if a field does not have a handler registered, but it does have a converter, use the converter instead. That might keep backwards compatibility somewhat and help with your use case (and the example would still work).

[natemcmaster]

So, before I implement anything else, here are some alternatives

Option 1 - behavior as I've implemented now, but add a flag which could be used to opt-out

# example of opting out
c = Converter(use_attrib_converters=False)
c.structure(...)

Option 2 - make this behavior an opt-in

# example of opting in
c = Converter(use_attrib_converters=True)
c.structure(...)

Option 3 - no new API, fallback to attr.ib converters if no handler is registered

Option 4 - fallback to attr.ib converters if no handler is found, but with an option to give attr.ib converters higher priority than registered handlers

c = Converter(prefer_attrib_converters=True)
c.structure(...)

Benefits of option 1 and 4 are that you could register an attribute converter which then calls cattr.structure.

---

I personally prefer option 1. As a user, this fits with what I was expecting to happen when registering an attr.ib converter.

I see the sense in the backwards compatible issue. What are your thoughts doing one of the other options in a 1.x update, and making using attr.ib converters the default in a new major version later?

[Tinche]

My order of preference would be 3, 4 and then 2. Option 1 by default breaks a use case that I'd like to support (referenced in my first comment).

[natemcmaster]

Thanks for the feedback, @Tinche. I've implemented option 3 from above and updated documentation. Let me know what you think!

[Tinche]

@natemcmaster Thanks! I have a couple busy days ahead so I can't look at this right away, but will as soon as I can!

[Tinche]

Left some comments, also don't forget to modify the changelog. Thanks!

[Tinche]

I don't think we need to use _fallback_to_passthru, we can know here and now if we will need to passthrough, right? If the handler is equal to converter._structure_default, the type is not is_generic and calling _structure_default raises an exception. It's a little dirty but should work.

Ideally the structure_default handler can be refactored to only contain the error logic, but that might be a different task.

[natemcmaster]

Sorry, I don't quite follow what you're proposing the code should do differently here.

[natemcmaster]

Updated per your feedback, except for the one piece I didn't understand.

Side note -- some thoughts about contributing to this project -- I'm not quite sure if I've done the right things, which will likely lengthen the feedback loop here. For example, the contributing.rst guide seems out of date with the changes to add poetry, use GitHub actions, and doesn't mention if I need to run formatters/linters (seems the CI may not check formatting.)

Anyways, lmk how this next draft works. Also, if you have a really specific idea of how something should work, feel free to edit my PR and merge without waiting for me.

[Tinche]

Side note -- some thoughts about contributing to this project -- I'm not quite sure if I've done the right things, which will likely lengthen the feedback loop here. For example, the contributing.rst guide seems out of date with the changes to add poetry, use GitHub actions, and doesn't mention if I need to run formatters/linters (seems the CI may not check formatting.)

You're right. I would be happy to merge a PR improving this :)

[Tinche]

I think this looks good, there's a small refactor I plan on making after this is merged to make it a little cleaner, but you shouldn't worry about it. Fix the conflicts and I shall merge.

Thanks a lot!

[natemcmaster]

Thanks @Tinche. I believe there are no more merge conflicts with the master branch. Thanks for the additional review :)

[Tinche]

Still can't merge for some reason

[natemcmaster]

@Tinche does squash merge work instead?

[Tinche]

Looks like it worked! Thanks.