-
Notifications
You must be signed in to change notification settings - Fork 4.3k
Add required field validation to nested structures #9463
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
base: v2
Are you sure you want to change the base?
Add required field validation to nested structures #9463
Conversation
false positives when the same field name appears at different levels with different requirement statuses.
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.
Looks good, main feedback is to differentiate this work from the constraints PR. We should also have a test for parameters that are expected to have required tags.
doc.style.new_paragraph() | ||
member_type_name = member_shape.type_name | ||
if member_type_name == 'structure': | ||
for sub_name, sub_shape in member_shape.members.items(): | ||
sub_shape.parent = member_shape |
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.
I don't love that we're assigning an arbitrary instance attribute that's not defined by our model classes. I suspect that this is trying to avoid exposing a new required
parameter to _do_doc_member
, but I would prefer plumbing it over setting arbitrary attributes.
@@ -289,10 +290,14 @@ def _do_doc_member(self, doc, member_name, member_shape, stack): | |||
doc.include_doc_string(docs) | |||
if is_tagged_union_type(member_shape): | |||
self._add_tagged_union_note(member_shape, doc) | |||
self._document_enums(member_shape, doc) | |||
required = self._check_if_required(member_shape, member_name) | |||
self._document_constraints(member_shape, doc, required=required) |
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.
I'm not sure it makes sense to couple the required field with constraints. Constraints define possible parameter values while required marks a parameter as required.
Issue #, if available: CLI-6123
Description of changes:
Example:

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.