-
Notifications
You must be signed in to change notification settings - Fork 1.7k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
add freeze
annotation
#17374
add freeze
annotation
#17374
Conversation
CodSpeed Performance ReportMerging #17374 will not alter performanceComparing Summary
|
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.
this is way better than how I was going about it, nice!
Left a few questions to better understand before approving
self.parameter_openapi_schema is not None | ||
and key in self.parameter_openapi_schema.get("properties", {}) | ||
): | ||
self.parameter_openapi_schema["properties"][key]["readOnly"] = True |
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.
does the UI do anything with this property?
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.
yes! you can't edit the value in the UI
and key in self.parameter_openapi_schema.get("properties", {}) | ||
): | ||
self.parameter_openapi_schema["properties"][key]["readOnly"] = True | ||
self.parameter_openapi_schema["properties"][key]["enum"] = [ |
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.
does this require the raw value to be JSON serializable or otherwise impose any restrictions on the types you can freeze
?
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.
hmm. yes I think values would need to be JSON serializable.. open to suggestions on how to handle that
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.
Yea that's probably fine for now (maybe we should add a ValueError
validation to freeze(...)
?) but I can see a world where more complex types are requested
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.
added in 05f4cab
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.
edit: nvm I misunderstood the use case
@classmethod | ||
def __get_pydantic_core_schema__( | ||
cls, source: type[Any], handler: GetCoreSchemaHandler | ||
) -> core_schema.CoreSchema: | ||
return core_schema.no_info_after_validator_function( | ||
cls, # Use the class itself as the validator | ||
core_schema.any_schema(), | ||
serialization=core_schema.plain_serializer_function_ser_schema( | ||
lambda x: x.unfreeze() # Serialize by unwrapping the value | ||
), | ||
) |
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.
this shouldn't matter as far as its use in RunnerDeployment
, but will make it behave more predictably in general
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!
closes #11073
this PR adds a
freeze
annotation type as described in the linked PR. adding a parameterized type (e.g.freeze[str]
) is not necessarythe field's value becomes a read-only
Enum
with only this member value (which be refined to more specifically patch the parameter schema)