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.

Sign up for GitHub

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

Jump to bottom

Document+test coverage for upconversion of new object fields #64

Merged
merged 14 commits into from
Jun 23, 2021
Merged

Document+test coverage for upconversion of new object fields #64

merged 14 commits into from
Jun 23, 2021

Conversation

kevingentile
Copy link
Contributor

@kevingentile kevingentile commented Jun 4, 2021 •
edited
Loading

Resolves #65

@CLAassistant
Copy link

CLAassistant commented Jun 4, 2021 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@kevingentile kevingentile mentioned this pull request Jun 4, 2021
Closed
@bartelink
Copy link
Collaborator

Does having a CartId option not map absence of a value to None ?

Mapping to Empty would be surprising and annoying

See also comments in #65 re how to make json.net reject cases where illegal nulls are supplied and/or how to otherwise control how missing values get rejected

@kevingentile
Copy link
Contributor Author

@bartelink I've updated the test and added an entry to the readme. If you don't find these changes useful feel free to modify or close the PR+Issue. Thanks!

bartelink
bartelink reviewed Jun 23, 2021
View reviewed changes
README.md Outdated Show resolved Hide resolved
@kevingentile @bartelink
Update README.md
27ccfe0 
Co-authored-by: Ruben Bartelink <[email protected]>
bartelink
bartelink reviewed Jun 23, 2021
View reviewed changes
README.md Outdated Show resolved Hide resolved
@kevingentile @bartelink
Update README.md
07239eb 
Co-authored-by: Ruben Bartelink <[email protected]>
bartelink
bartelink reviewed Jun 23, 2021
View reviewed changes
README.md Outdated Show resolved Hide resolved
bartelink
bartelink reviewed Jun 23, 2021
View reviewed changes
README.md Outdated Show resolved Hide resolved
@kevingentile @bartelink
Update README.md
9ce1f32 
Co-authored-by: Ruben Bartelink <[email protected]>
bartelink
bartelink reviewed Jun 23, 2021
View reviewed changes
README.md Outdated Show resolved Hide resolved
kevingentile and others added 2 commits June 23, 2021 10:38
@kevingentile @bartelink
Update README.md
1fc20b0 
Co-authored-by: Ruben Bartelink <[email protected]>
@kevingentile @bartelink
Update README.md
a127e7f 
Co-authored-by: Ruben Bartelink <[email protected]>
bartelink
bartelink reviewed Jun 23, 2021
View reviewed changes
README.md Outdated Show resolved Hide resolved
@kevingentile @bartelink
Update README.md
76f797a 
Co-authored-by: Ruben Bartelink <[email protected]>
bartelink
bartelink reviewed Jun 23, 2021
View reviewed changes
README.md Outdated Show resolved Hide resolved
@kevingentile @bartelink
Update README.md
b26c37b 
Co-authored-by: Ruben Bartelink <[email protected]>
bartelink
bartelink reviewed Jun 23, 2021
View reviewed changes
README.md Outdated Show resolved Hide resolved
@kevingentile @bartelink
Update README.md
be8a13a 
Co-authored-by: Ruben Bartelink <[email protected]>
@bartelink bartelink changed the title add failing test for unexpected null property deserialization Document and add test coverage for upconversion of new object fields Jun 23, 2021
@bartelink bartelink changed the title Document and add test coverage for upconversion of new object fields Document+test coverage for upconversion of new object fields Jun 23, 2021
@bartelink
Copy link
Collaborator

This is looking great, thanks so much for taking the time and doing a clean example and nice docs

@bartelink
Copy link
Collaborator

Looks great now, shall I merge ?

@bartelink bartelink merged commit 7762d84 into jet:master Jun 23, 2021
@bartelink
Copy link
Collaborator

Thanks for your contribution @kevingentile - triply so as it's a) a test and b) adds documentation !

@kevingentile kevingentile deleted the null-deserialize branch September 20, 2022 18:04
@bartelink bartelink mentioned this pull request Jan 25, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bartelink bartelink bartelink left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Deserialization of null to non-optional Record property
3 participants
@kevingentile @CLAassistant @bartelink