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

Deserialization improvements #84

Merged
merged 6 commits into from
Jun 24, 2020

Conversation

jamalex
Copy link
Member

@jamalex jamalex commented Jun 11, 2020

Summary

Some records at times fail to deserialize on KDP (due to missing data, specifically foreign key referents). Added some tests that uncovered a potential cause, and fixed that, and then also added a column called deserialization_error on the Store model to track issues with deserialization, to support debugging and to be able to skip them for a "lighter weight" deserialization as desired.

TODO

  • Have tests been written for the new code?
  • Has documentation been written/updated?
  • New dependencies (if any) added to requirements file

Issues addressed

Addresses issues with bulk deserialization seen on KDP.

@codecov-commenter
Copy link

codecov-commenter commented Jun 11, 2020

Codecov Report

Merging #84 into master will decrease coverage by 0.61%.
The diff coverage is 83.33%.

@@            Coverage Diff             @@
##           master      #84      +/-   ##
==========================================
- Coverage   83.46%   82.85%   -0.62%     
==========================================
  Files          31       37       +6     
  Lines        2316     2385      +69     
  Branches      297      301       +4     
==========================================
+ Hits         1933     1976      +43     
- Misses        286      309      +23     
- Partials       97      100       +3     

Copy link
Member

@bjester bjester left a comment

Choose a reason for hiding this comment

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

The only thing that confused me initially was that _deserialize_store_model catches and re-raises the error.

@jamalex
Copy link
Member Author

jamalex commented Jun 17, 2020

The only thing that confused me initially was that _deserialize_store_model catches and re-raises the error.

Yeah, not immediately obvious from looking at the code what's going on there. I'll add a couple more comments to that section.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants