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

Fix frozenset deserialization. #342

Merged
merged 1 commit into from
Feb 4, 2022

Conversation

pchanial
Copy link
Contributor

@pchanial pchanial commented Feb 1, 2022

Closes #341

Note that there is another unit test failure : the deserialization of BigInt is not a BigInt instance. For this test case, I've explicitly disabled the new type check since it is another issue.

constraints: Tuple[Constraint, ...]
value_method: DeserializationMethod

def deserialize(self, data: Any) -> Any:
Copy link
Owner

Choose a reason for hiding this comment

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

There is no need to rewrite a complete method for frozenset; ListMethod should be reused, as it's already the case for variadic tuple

Copy link
Owner

Choose a reason for hiding this comment

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

By the way, I think super() was not supported (in Cython generation) when I wrote VariadicTupleMethod, but this one, as well as FrozenSetMethod should simply inherit ListMethod and use for example return tuple(super().deserialize(data))

Copy link
Contributor Author

@pchanial pchanial Feb 2, 2022

Choose a reason for hiding this comment

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

After inspection, it is not straightforward, since VariadicTupleMethod could inherit from ListCheckOnlyMethod or ListMethod.
It looks like all collection deserialization methods could benefit from only checking their item values, so I'd say that it would be better to do some refactoring here.

Copy link
Owner

Choose a reason for hiding this comment

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

You're right, it was not a question of super() support … I was too quick in my response. So composition should be used the same way VariadicTupleMethod does.

tests/unit/test_deserialization_serialization.py Outdated Show resolved Hide resolved
tests/unit/test_deserialization_serialization.py Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

Codecov Report

Merging #342 (7c47d6d) into master (af1bf4a) will decrease coverage by 0.02%.
The diff coverage is 80.95%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #342      +/-   ##
==========================================
- Coverage   86.13%   86.11%   -0.03%     
==========================================
  Files          67       67              
  Lines        5879     5898      +19     
  Branches     1218     1222       +4     
==========================================
+ Hits         5064     5079      +15     
- Misses        596      599       +3     
- Partials      219      220       +1     
Impacted Files Coverage Δ
apischema/deserialization/methods.py 84.51% <76.47%> (-0.21%) ⬇️
apischema/deserialization/__init__.py 98.98% <100.00%> (+<0.01%) ⬆️

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 af1bf4a...7c47d6d. Read the comment docs.

@wyfo
Copy link
Owner

wyfo commented Feb 3, 2022

@pchanial
Actually, I can merge this PR like this, and do the small refactoring myself if you're ok with that. I would like to release the patch tomorrow morning (I don't like having a bug more than 24h in the issues).
If you want to make the change yourself, I will wait, no worry.

pchanial pushed a commit to pchanial/apischema that referenced this pull request Feb 4, 2022
@pchanial pchanial force-pushed the fix-frozenset-deserialization branch from 42bb0ff to 90c4687 Compare February 4, 2022 01:41
@pchanial pchanial force-pushed the fix-frozenset-deserialization branch from 90c4687 to 2d54825 Compare February 4, 2022 01:50
@pchanial
Copy link
Contributor Author

pchanial commented Feb 4, 2022

I had made the change before your last comment, so I just pushed it. I have started looking at ways to refactor this part but the tests fail with cython. I'll make another PR so we can discuss it.

@wyfo wyfo merged commit 5450280 into wyfo:master Feb 4, 2022
@pchanial pchanial deleted the fix-frozenset-deserialization branch February 4, 2022 08:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix frozenset deserialization
3 participants