Skip to content

StructType field optional by default #592

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

Merged
merged 7 commits into from
Apr 17, 2024

Conversation

MehulBatra
Copy link
Contributor

@MehulBatra MehulBatra commented Apr 8, 2024

  • Type change default = false
  • Updated test cases to align with recent changes in the codebase (Unit + Integration)
  • mkdoc changes to update required field default value

@MehulBatra MehulBatra changed the title StructType field required optional by default StructType field optional by default Apr 8, 2024
Copy link
Contributor

@Fokko Fokko 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 @MehulBatra Left one small suggestion.

Could you also remove required=False from mkdocs/docs/api.md. Since this is now default, I think we should remove it to make the docs less verbose.

@MehulBatra
Copy link
Contributor Author

MehulBatra commented Apr 17, 2024

Looks good @MehulBatra Left one small suggestion.

Could you also remove required=False from mkdocs/docs/api.md. Since this is now default, I think we should remove it to make the docs less verbose.

On it, pushing changes for the same.
thank you!

@MehulBatra MehulBatra requested a review from Fokko April 17, 2024 11:36
@MehulBatra MehulBatra force-pushed the nested_field_struct_type branch from 87aca9d to e354f86 Compare April 17, 2024 12:49
@MehulBatra MehulBatra force-pushed the nested_field_struct_type branch from e354f86 to 9592d12 Compare April 17, 2024 13:55
Copy link
Contributor

@Fokko Fokko left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for working on this @MehulBatra

@Fokko
Copy link
Contributor

Fokko commented Apr 17, 2024

@MehulBatra I've removed an unrelated change, I think #612 addresses the issue that you ran into

@MehulBatra
Copy link
Contributor Author

@MehulBatra I've removed an unrelated change, I think #612 addresses the issue that you ran into

sounds good!

@Fokko Fokko merged commit aa850ef into apache:main Apr 17, 2024
@Fokko
Copy link
Contributor

Fokko commented Apr 17, 2024

Thanks for fixing this @MehulBatra Much appreciated!

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