Closed huwcbjones closed 11 months ago
@lovasoa the tests appear to also be broken on master due to bitrot, have fixed those. Also added documentation
Have pushed a better fix for the mypy test failure
Thanks for the cleanup !
Last failing test seems to be some ordering thing that I have no idea how to fix 😬
I've just pushed a commit that refactors TestFieldForSchema.assertFieldsEqual
to make it somewhat more robust.
It could probably use a little more cleanup and review. (E.g. to make sure I haven't inadvertently made the check too lenient.)
The problem was with the order of union fields. But are you sure we don't care about it? If there is a logic of "first field that works", then it won't behave the same. I think the actual problem is in the code, not the tests.
The problem was with the order of union fields. But are you sure we don't care about it? If there is a logic of "first field that works", then it won't behave the same.
You're right, of course. Crap.
(Though I still think the rest of the refactor to assertFieldsEqual
is an improvement over the previous implementation, which mostly relied on comparing reprs.)
I was writing a comment about comparing the values to themselves, but I see you realized it yourself. But the point is: it's much more complicated and error prone that way. Can we go back to just the reprs ? It was very simple and worked well. If it's not broken, don't fix it :stuck_out_tongue:
Here's the issue with the Union field ordering. (I think.)
It has to do with the fact that typing
caches some types.
>>> from typing import Optional, Union
>>> Optional[Union[int, str]] is Optional[Union[str, int]]
True
# Even though...
>>> Union[int, str] is Union[str, int]
False
The failure of our [test_optional_multiple_types] is exercised by this change in pytest-mypy-plugins
1.11.0.
The cause of all this is that, while our Union
field is order-dependent, union types semantically do not depend on the order of their arguments. (Union[str, int]
and Union[int, str]
refer to the same type.)
I'm not sure what the fix is.
Wow, that is strange ! If there is really no way to extract the original order of the union members, then we don't have any choice but to special-case our tests to allow any order for the members of union_fields
Can we go back to just the reprs ?
Okay, I've reverted my changes.
I've left my reversion of e8e29b9a45ad76d7894910b9350cb025071c28b8, and partial reversion of cbad82bdce3d8d7605f2767c050ca1f539d7d5b5 since both of those attempted to fix tests by making them insensitive to Union field order.
Ok, thanks to both of you. I'm going to release a new version
Wow, that is strange ! If there is really no way to extract the original order of the union members, then we don't have any choice but to special-case our tests to allow any order for the members of
union_fields
But the tests arewere correct! (They are correctly revealing a problem.) You correctly pointed out that the behavior of our Union
field depends on the ordering. Our support for union types is broken if union types don't preserve argument order.
Here's a clearer explanation of what's going on...
Typing
maintains a cache of constructed types. The cache is (essentially) keyed on the arguments to the type.
Union[str, int]
and Union[int, str]
have different arguments ((str, int) != (int, str)
), so they evaluate to different instances.
But, even though they are different instances, Union[str, int] == Union[int, str]
. Union types don't consider argument order when computing equality.
So Union[Union[int, str]]
will return the same (cached) instance as Union[Union[str, int]]
, because the arguments to the outer Union
compare equal ((Union[int, str],) == (Union[str, int],)
). (Optional[Union[int, str]]
is equivalent to Union[Union[int, str], type(None)]
.)
Brainstorming...
Maybe we could implement our own custom union type that would bypass typing
s type cache and so preserve the argument ordering but otherwise work the same as typing.Union
. (I'm not sure how easy that is to do in a way that will work with all supported python versions.)
We could maybe somehow monkeypatch typing.Union
to make it bypass the type cache. (Yuck. Again, not sure how easy this is to do in a python-version-independent say.)
You are right ! You can make such a patch, but personally, I'd just wait to see if anyone needs it. This theoretically can be a problem, but I'm not sure a fix is urgently needed
Ok, thanks to both of you, I'm going to release a new version
@lovasoa The Union type ordering issue has nothing to do with the original point of the PR, so I don't object to making a release to get the include_non_init
option out.
But, our handling of union types is quite broken. (And I think it always has been?)
The tests were correct in checking the order of union_fields
. Your last commit should be reverted in master and an issue created.
Here's a simple demo to illustrate the issue. (Put this in broken.py
then run python broken.py
.)
from typing import Optional, Union
from marshmallow_dataclass import dataclass
# Comment out the next line and the assert will pass, otherwise it will fail
Optional[Union[str, int]]
@dataclass
class Test:
x: Optional[Union[int, str]]
assert Test.Schema().load({"x": "42"}) == Test(x=42)
I agree we should create an issue and document the current clunky behavior, but I don't think we should re-break the tests on master
I agree we should create an issue and document the current clunky behavior, but I don't think we should re-break the tests on master
The failing tests could be marked xfail
don't you think it's better to still test that the union members are all here and correct than giving up on testing union altogether ?
don't you think it's better to still test that the union members are all here and correct than giving up on testing union altogether ?
We could do both. Mark the existing failing tests as expected failures. Add a new test (or tests) to check things that currently work that were checked by the failing tests.
I think the xfail tests are important as reminders of what is broken. Since we know there's a problem, there should be a test that exercises it.
Good, I'm fine with that !
Updates #60
This adds the ability to include non init-ed fields into the schema by using a new Meta option ,
include_non_init
.I don't think it's the best way to address #60, but it is a very small code change to get the functionality working.