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

Adds extended JSONEncoder that can serialize pydantic models. #8

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

strue36
Copy link

@strue36 strue36 commented Apr 1, 2022

Fixes the issue described in #7

@codecov
Copy link

codecov bot commented Apr 4, 2022

Codecov Report

Merging #8 (2913c0c) into master (2662ad7) will decrease coverage by 3.31%.
The diff coverage is 100.00%.

❗ Current head 2913c0c differs from pull request most recent head b32e08f. Consider uploading reports for the commit b32e08f to get more accurate results

@@            Coverage Diff             @@
##           master       #8      +/-   ##
==========================================
- Coverage   89.68%   86.37%   -3.32%     
==========================================
  Files          30       30              
  Lines         931      925       -6     
==========================================
- Hits          835      799      -36     
- Misses         96      126      +30     
Impacted Files Coverage Δ
rath/json/encoders.py 100.00% <100.00%> (ø)
rath/links/aiohttp.py 62.29% <100.00%> (-6.94%) ⬇️
tests/test_json.py 100.00% <100.00%> (ø)
rath/links/validate.py 59.15% <0.00%> (-37.28%) ⬇️
rath/links/parsing.py 55.00% <0.00%> (-33.89%) ⬇️
tests/test_validation.py 73.33% <0.00%> (-23.22%) ⬇️
rath/links/split.py 87.87% <0.00%> (-12.13%) ⬇️
rath/links/base.py 71.64% <0.00%> (-10.18%) ⬇️
rath/links/dictinglink.py 75.00% <0.00%> (-7.15%) ⬇️
... and 23 more

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 2662ad7...b32e08f. Read the comment docs.

@jhnnsrs
Copy link
Owner

jhnnsrs commented Apr 4, 2022

Thanks Sean!
I had a similar implementation of this in a dedicated link (Dicting Link), which stirs up the question of what should go into links and what would be considered common functionality to be implemented directly in a terminating link. Do you have any suggestions in what would be considered a spec of a terminating link? I was thinking that maybe the terminating link should only assert that everything is json serializable and raise an error if not, leaving the complex json encoding to another link? Whats your idea?

@strue36
Copy link
Author

strue36 commented Apr 6, 2022

I think this also works as a dedicated link. I'm not sure whether it should be a DictingLink as we probably want this link to handle other conversions from complex types to objects that are json serialisable (Such as datetime -> str). It looks like ParsingLink covers this though.

One thing that a custom JSONEncoder does bring is that it handles recursion nicely. I think that the current implementation of DictingLink will only handle top level BaseModels, but will skip over nested types such as List[BaseModel]. (It looks like it also throws away any variables that aren't a BaseModel)

If this conversion is intended to be handled by a ParsingLink this should probably be included in the rath/turms documentation here as it is likely that people will have pydantic models in their turms generated code. It should also be documented that the TerminatingLink expects all variables to be in a specific format. I think your suggestion of validating

Do you think the AIOHttpLink should expose a way to modify the default values of the ClientSession? Even if the conversion of variables is handled in a ContinuationLink, people may want to use a different json serializer such as ujson.

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.

2 participants