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

Pydantic support #200

Merged
merged 2 commits into from
Sep 10, 2020
Merged

Pydantic support #200

merged 2 commits into from
Sep 10, 2020

Conversation

gmcrocetti
Copy link
Contributor

Fixes #146 .

Changes proposed in this pull request:
This PR introduces pydantic as one of the available models. I'd like to hear your opinion before proceeding with docs.

Attention: @prkumar

@codecov
Copy link

codecov bot commented Aug 7, 2020

Codecov Report

Merging #200 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #200   +/-   ##
=======================================
  Coverage   99.91%   99.91%           
=======================================
  Files          43       44    +1     
  Lines        2355     2398   +43     
  Branches      173      176    +3     
=======================================
+ Hits         2353     2396   +43     
  Misses          2        2           
Impacted Files Coverage Δ
uplink/converters/__init__.py 100.00% <100.00%> (ø)
uplink/converters/pydantic_.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 4c9a380...07edb49. Read the comment docs.

@prkumar prkumar self-requested a review August 10, 2020 21:40
@prkumar prkumar self-assigned this Aug 10, 2020
@gmcrocetti
Copy link
Contributor Author

@prkumar . Don't know why it's failing. Tried to reproduce locally and it worked with all python versions.

Pipfile Outdated Show resolved Hide resolved
@prkumar
Copy link
Owner

prkumar commented Aug 11, 2020

@prkumar . Don't know why it's failing. Tried to reproduce locally and it worked with all python versions.

Looking at the job log in Travis (link), I see that pytest is failing:

ERROR: usage: py.test [options] [file_or_dir] [file_or_dir] [...]
py.test: error: unrecognized arguments: --cov-config .coveragerc --cov=uplink

I doubt this is an issue with the Python code you added, and since the tests are passing in master, I don't think this is an issue with a new py.test version either. If I had to guess, I think it may be the line change in Pipfile?

@gmcrocetti gmcrocetti force-pushed the pydantic-support branch 2 times, most recently from 83fcf71 to 4a17df1 Compare August 11, 2020 01:47
@gmcrocetti
Copy link
Contributor Author

@prkumar . Don't know why it's failing. Tried to reproduce locally and it worked with all python versions.
Looking at the job log in Travis (link), I see that pytest is failing:

ERROR: usage: py.test [options] [file_or_dir] [file_or_dir] [...]
py.test: error: unrecognized arguments: --cov-config .coveragerc --cov=uplink

I doubt this is an issue with the Python code you added, and since the tests are passing in master, I don't think this is an issue with a new py.test version either. If I had to guess, I think it may be the line change in Pipfile?

Yes ! I've tried to run/debug on my machine and it worked (ohhh, what a surprise, hahaha). I'm also sure it's related with the Pipfile modification, but still not quite sure how...

@gmcrocetti
Copy link
Contributor Author

Tests ok. Any updates @prkumar ? :)

Copy link
Owner

@prkumar prkumar left a comment

Choose a reason for hiding this comment

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

Great work, @gmcrocetti! This looks good to me. You mentioned adding documentation for this. Is this still planned?

@gmcrocetti
Copy link
Contributor Author

Great work, @gmcrocetti! This looks good to me. You mentioned adding documentation for this. Is this still planned?

Thanks ! :) Yeap, hope I can finish this week.

@gmcrocetti
Copy link
Contributor Author

Docs updated ! Left a comment asking for your opinion. Ready for review ! 👍

docs/source/index.rst Outdated Show resolved Hide resolved
docs/source/user/serialization.rst Outdated Show resolved Hide resolved
@gmcrocetti gmcrocetti changed the title WIP: Pydantic support Pydantic support Sep 7, 2020
Copy link
Owner

@prkumar prkumar left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for addressing this, @gmcrocetti. If you want, please add yourself to the AUTHORS file!

@gmcrocetti
Copy link
Contributor Author

LGTM. Thanks for addressing this, @gmcrocetti. If you want, please add yourself to the AUTHORS file!

Oh, thanks for that ! :)

@prkumar
Copy link
Owner

prkumar commented Sep 10, 2020

@gmcrocetti - Last-minute ask: could you squash your commits before I merge? Sorry for the hassle!

…e client's behavior into pydantic's models

feat(setup): Add pydantic as extra package and update pipenv
@gmcrocetti
Copy link
Contributor Author

@gmcrocetti - Last-minute ask: could you squash your commits before I merge? Sorry for the hassle!

No problem. Should be fixed.

@prkumar prkumar merged commit 2f4260d into prkumar:master Sep 10, 2020
@prkumar prkumar added this to the v0.9.2 milestone Sep 10, 2020
@JJia-Perl
Copy link

Does this PR adds support for GenericModel?

@prkumar prkumar mentioned this pull request Oct 17, 2020
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.

Consider adding "pydantic" as built-in serialization / deserialization option
3 participants