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

Nullable values #100

Merged
merged 3 commits into from
Apr 30, 2020
Merged

Nullable values #100

merged 3 commits into from
Apr 30, 2020

Conversation

filak-sap
Copy link
Contributor

No description provided.

@filak-sap filak-sap force-pushed the nullable_values branch 2 times, most recently from c776c12 to 4f06094 Compare April 29, 2020 11:26
@codecov-io
Copy link

codecov-io commented Apr 29, 2020

Codecov Report

Merging #100 into master will increase coverage by 0.40%.
The diff coverage is 95.12%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #100      +/-   ##
==========================================
+ Coverage   92.41%   92.81%   +0.40%     
==========================================
  Files           6        6              
  Lines        2439     2465      +26     
==========================================
+ Hits         2254     2288      +34     
+ Misses        185      177       -8     
Impacted Files Coverage Δ
pyodata/v2/service.py 90.45% <75.00%> (ø)
pyodata/v2/model.py 93.53% <100.00%> (+0.59%) ⬆️

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 f9144b3...ce0b6cf. Read the comment docs.

@filak-sap filak-sap requested a review from phanak-sap April 29, 2020 18:45
@filak-sap filak-sap force-pushed the nullable_values branch 2 times, most recently from 445b94e to cc8c71a Compare April 29, 2020 21:59
I came a cross this difference during testing of SAP#95

I decided to set tzinfo of JSON DateTimes to simplify my tests and it
also make sense to me to have the values configured the same way.
Tests will appear in the next commit. I don't find it usefull
to add the tests here.
Only a declared property knows if it accepts Null or not. I was playing
with nullable Type but than I would have to have also nullable
TypeTraits and there result would be a complete mess. Therefore I decided
to move the responsibility to serialize/deserialize the declared
property. The decision led to a simpler code which is a good sign.

I could have used a better exception type but we currently evaluating
the best exception strategies, so I used a generic type.

Part of the issue SAP#95
Copy link
Contributor

@phanak-sap phanak-sap left a comment

Choose a reason for hiding this comment

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

Looks good to me, comments resolved.

@filak-sap filak-sap merged commit 7972f31 into SAP:master Apr 30, 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.

None yet

3 participants