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

remove all instances of Required.AllowNull #1078

Merged
merged 1 commit into from
Mar 4, 2024
Merged

Conversation

andrewheumann
Copy link
Member

@andrewheumann andrewheumann commented Mar 4, 2024

BACKGROUND:

  • In a number of cases, our NJsonSchema-generated classes reflected an unintentional strange state: that of being required but possibly null. This is rarely practically useful: in nearly all cases we want a property to either be required or to permit null, but not both.

DESCRIPTION:

  • Adapts all usage of the Required.AllowNull attribute to be either Required.Always, or not required. Since these values could always be null anyway, the code is generally able to tolerate their "nullness" — the difference is just whether or not we get a serialization failure if a null is omitted from the json. For example:
{
   Perimeter: [...],
   Voids: null
}

is a valid Profile, but

{
   Perimeter: [...],
}

will throw a deserialization error, even though the code can handle it.

TESTING:

  • All tests continue to pass.

This change is Reviewable

@andrewheumann andrewheumann requested a review from ikeough March 4, 2024 19:36
Copy link
Contributor

@ikeough ikeough left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: :shipit: complete! 1 of 1 approvals obtained

@andrewheumann andrewheumann merged commit a5e8195 into master Mar 4, 2024
2 checks passed
@andrewheumann andrewheumann deleted the no-allow-null branch March 4, 2024 22:20
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