-
Notifications
You must be signed in to change notification settings - Fork 81
✨ Add nullable key to needs_fields items
#1613
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
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1613 +/- ##
==========================================
+ Coverage 86.87% 88.24% +1.36%
==========================================
Files 56 70 +14
Lines 6532 9929 +3397
==========================================
+ Hits 5675 8762 +3087
- Misses 857 1167 +310
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| If specified, the field value will be validated against this schema when needs are parsed, see :ref:`schema_validation` for more details. | ||
| By default the field schema will be ``{"type": "string"}``. | ||
| - ``nullable``: If set to ``True``, the field can be set to ``None`` (optional), e.g. if no value is specifically given and no default applies, | ||
| If ``False``, the field must have a value (either explicitly set or via default/predicates), otherwise the need is invalid and will not be created. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How would users know the default for nullable? This depends on the field, we should state this somewhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Obviously, this is only for inherited core fields, but yes we should show their "full" configuration in the needs_fields documentation.
Maybe leave this for a follow-up PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
d230b83 to
975e42a
Compare
ubmarco
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for the update
This pull request introduces support for a
nullableproperty in need field definitions, allowing fields to be explicitly marked as allowing or disallowing unset (None) values. The changes affect the configuration, schema validation, and documentation, and include updates to both the codebase and the test suite to ensure correct handling and enforcement of field nullability.For example: