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

FIX: Give Optionals a default value #818

Merged
merged 1 commit into from
Sep 26, 2024

Conversation

mferrera
Copy link
Collaborator

Resolves #814
Resolves #815

I still have a bit uncertainty about some fields. In some cases we give optional lists a default value of an empty list, in some cases it's null.

A few instances I added the Field or sorted the keyword arguments to it to make the default show first.

I'm working on a test that will run through all the models and fail if an optional doesn't provide a default value... but figured I can put this up before that is working.

Comment on lines 106 to 108
checksum_md5: Optional[str] = Field(
default=None, examples=["kjhsdfvsdlfk23knerknvk23"]
)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

A test requires that validation cannot occur without checksum, still listed as Optional. Is this a backward compatibility thing patch-handled in Sumo?

@mferrera mferrera force-pushed the ensure-optionals-have-defaults branch from bb5b7a0 to b2d2801 Compare September 26, 2024 11:57
@mferrera mferrera self-assigned this Sep 26, 2024
@mferrera mferrera marked this pull request as ready for review September 26, 2024 12:03
@mferrera mferrera force-pushed the ensure-optionals-have-defaults branch from b2d2801 to b3f3818 Compare September 26, 2024 12:21
Copy link
Collaborator

@tnatt tnatt left a comment

Choose a reason for hiding this comment

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

Nice 👍 I don't really have a strong opinion on what's best regarding the fields with optional list.. but lean towards having them defaulted to None

@mferrera mferrera merged commit c34f6dd into equinor:main Sep 26, 2024
13 checks passed
@mferrera mferrera deleted the ensure-optionals-have-defaults branch September 26, 2024 12:44
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.

Ensure all Optional fields have a default Make num_columns and num_rows actually optional
2 participants