-
Notifications
You must be signed in to change notification settings - Fork 49
fix: restore payload_builder wrapping for dict schemas #910
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -48,6 +48,9 @@ def validate_schema(schema, obj, msg_prefix): | |
| # Handle plain Python types (e.g. str, int) via msgspec.convert | ||
| elif isinstance(schema, type): | ||
| msgspec.convert(obj, schema) | ||
| # Handle plain dict schemas (e.g. from downstream payload builders) | ||
| elif isinstance(schema, dict): | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What's the plan for plain-dict schemas going forward? Will they be removed in favour of msgspec ones? Will this block need to be updated to convert them to msgspec schemas?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Eventually yes since all plain dict seems get convert to voluptuous. For cross compatibility we will still need to support them for the time being |
||
| voluptuous.Schema(schema)(obj) | ||
| else: | ||
| raise TypeError(f"Unsupported schema type: {type(schema)}") | ||
| except ( | ||
|
|
||
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 I understand why we're adding these fields for plain-dict schemas. Wouldn't the incoming schemas have them if they are 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.
This was existing behavior, this is from the v18.1 of taskgraph : https://github.com/taskcluster/taskgraph/blob/18.1.0/src/taskgraph/transforms/task.py#L437 . We can eventually drop support for this once everything is converted over
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.
Ah, thank you - I was unable to dig up that reference. sgtm