-
Notifications
You must be signed in to change notification settings - Fork 456
chore(python-sdk): literal defaults #49
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
Conversation
117f6ef
to
097f6fd
Compare
Set the event type and message role using pydantic Field and disable setting in the constructor to help prevent accidental misuse while reducing unnecessary boilerplate code. Replace Role type alias with an Enum to eliminate repeating string literals and align with EventType. Use Field(min_length=1) to validate TextMessageContentEvent delta, simplifying the code. Fixes ag-ui-protocol#41
097f6fd
to
9e12e6b
Compare
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.
Pull Request Overview
This PR refactors the SDK to enforce literal defaults for event types and message roles using pydantic Fields with init=False, reducing boilerplate and minimizing misuse. Key changes include:
- Removing explicit specification of role and type in message/event constructors.
- Replacing Role type alias with a Role Enum.
- Adding Field(min_length=1) validation for TextMessageContentEvent delta.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
python-sdk/tests/test_types.py | Removed redundant role entries from tests for message serialization |
python-sdk/tests/test_events.py | Removed explicit type and role fields from event tests |
python-sdk/tests/test_encoder.py | Removed explicit type field in event creation for encoding tests |
python-sdk/ag_ui/core/types.py | Updated message role definitions to use the Role Enum and Field with init=False |
python-sdk/ag_ui/core/events.py | Updated event type definitions to use Field with literal defaults and init=False |
Comments suppressed due to low confidence (1)
python-sdk/tests/test_events.py:367
- With the use of Field(min_length=1) for the 'delta' field, pydantic now raises a ValidationError instead of a ValueError when an empty string is provided. Update the test in test_validation_constraints to expect ValidationError instead of ValueError.
TextMessageContentEvent(
@NathanTarbert @mme could you have a look at this, I would like to leverage it for the pydantic-ai adapter. |
Hi Steven, All in all, I very much like this change. I can't imagine a scenario where we want these events being instantiated with a different type than their class type. My only concern is the breaking api change here for existing code. 100% agree that the change should happen, I just don't have the context on how many people this would break things for (not that the fix wouldn't also be simple relative, just deleting some arguments when instantiating). @mme any thoughts here? At worst we can just make the override optional and deprecate it, but I feel like we're early enough to skip that unless you think it will cause undue trouble for people. |
Yes its breaking change, but I would agree it's early enough and simple enough to fix, just remove the values, so it shouldn't be too much of a burden for anyone. If you wanted to be conservative we could remove init=false and that would be compatible. I wouldn't encourage that as we all know people copy existing examples, so that would just allow this pattern to persist. |
Yeah, let's make it optional (and still remove it from the examples). After talking with @mme It seems like we have enough people using the python sdk already enough that breaking it unannounced would cause enough pain to make it worth avoiding doing that. Is there an effective way to mark it deprecated so we can start coaxing people off of it? |
Thanks @maxkorp I'll get that done today. |
Remove `init=False` from default literals to improve compatibility with existing code. Minimise changes by using literal strings instead of enum.
e45d60c
to
049bb62
Compare
I've minimised the changes so it should now be compatible with existing code @maxkorp |
Set the event type and message role using pydantic Field and disable setting in the constructor to help prevent accidental misuse while reducing unnecessary boilerplate code.
Replace Role type alias with an Enum to eliminate repeating string literals and align with EventType.
Use Field(min_length=1) to validate TextMessageContentEvent delta, simplifying the code.
Fixes #41