Fix include_subclasses strategy with a generic parent and tagged_union#683
Conversation
60feae8 to
af4565d
Compare
|
Thanks, I'll take a look when I get some free cycles. Can you get me a more detailed error message on that flaky test in the meantime? |
|
Here is the failing test output. |
|
Cool, thanks. Can you rebase, I reworked those tests. |
af4565d to
a198b33
Compare
|
I rebased. All tests now pass. |
|
Looks great to me. I'm away from a computer until tomorrow, what happens if we don't substitute the tag generator? |
|
Without substituting tag_generator, the test fails like this: src/cattrs/strategies/_unions.py:58: in configure_tagged_union
src/cattrs/strategies/_unions.py:19: in default_tag_generator
return typ.__name__
typ = tests.strategies.test_include_subclasses.test_parents_with_generics_tagged_union.<locals>.GenericParent[typing.Any]
/usr/lib/python3.9/typing.py:711: in __getattr__
raise AttributeError(attr)
E AttributeError: __name__So it seems like a good idea to do this in the default tag generator, but like I said, I'm not sure if that approach means making similar changes in many more places. What code 'officially' supports generic types? Should all code support generic types and any code that breaks be fixed when we notice it? Should 'higher level' / user facing code apply |
|
Got it. Hm, it only fails on 3.9 for me. I wouldn't change it globally, especially since in a few months 3.9 goes away. |
|
You're right, I didn't realize that. I don't personally care about supporting python 3.9, so that's fine by me! |
|
Cool. Thanks for the contribution! |
This extends the fix in #650 to the tagged union strategy. I reported the issue in #682.
Without this, using include_subclasses with tagged_union and a generic parent class raises the error
AttributeError: __subclasses__. Did you mean: '__subclasscheck__'?.Note: I don't know whether to edit
default_tag_generatorto sayreturn (typing.get_origin(typ) or typ).__name__. It doesn't break any tests, but I'm not sure if there are cases where it wouldn't be appropriate, or what else ought to be changed if this is. For now I left it as a tag_generator override at the callsite, but it would be better for users not to have to pass this. Please advise.The tests don't pass for me locally on the main branch (regardless of my change), but the failure is in a test that doesn't use these strategies and all the tests passed when I rebased my fix on 25.1.1, so I hope I'm just confused about something unrelated. (The failure is
tests/test_gen_dict.py::test_individual_overrides - assert 'Factory' not in "{'a': ('', ... '_b': None}")(ETA: the tests now pass, see comments)