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

error: allow user and external group ids #42

Merged
merged 3 commits into from
Feb 11, 2025

Conversation

sam-golioth
Copy link
Contributor

The V2 Error group field only accepted header.GroupId, which would lead to validation errors when smpclient received v2 error responses from SMP groups beyond the standard set. This change allows creating ErrorV2 instances for both User Groups defined in the tree (i.e. Intercreate) as well as for User Groups defined outside of the tree.

Let me know if there's a more preferred way to solve this problem!

Copy link
Owner

@JPHutchins JPHutchins left a comment

Choose a reason for hiding this comment

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

Thanks! It came up before, and we added this GroupIdField: ffea1a5

It's nice because it has the explicit behavior of attempting to parse as GroupId, then UserGroupId, before falling back to int.

Sorry to create maintenance work, but perhaps it makes sense to relocate that GroupIdField definition and then use it in the error.py file?

@sam-golioth
Copy link
Contributor Author

sam-golioth commented Feb 11, 2025

Thanks! It came up before, and we added this GroupIdField: ffea1a5

It's nice because it has the explicit behavior of attempting to parse as GroupId, then UserGroupId, before falling back to int.

Sorry to create maintenance work, but perhaps it makes sense to relocate that GroupIdField definition and then use it in the error.py file?

Can do! I'm thinking header.py makes the most sense for where to put it. AnyGroupId is only used in Unions, so we could remove it and use GroupIdField in place of the Unions instead:

group_id: Union[AnyGroupId, GroupId, UserGroupId]

_GROUP_ID: ClassVar[smpheader.GroupId | smpheader.UserGroupId | smpheader.AnyGroupId]

What do you think?

@sam-golioth sam-golioth force-pushed the patch-1 branch 2 times, most recently from 908e6c2 to 8e23dae Compare February 11, 2025 01:44
sam-golioth and others added 3 commits February 10, 2025 20:52
Signed-off-by: Sam Friedman <sam@golioth.io>
Signed-off-by: Sam Friedman <sam@golioth.io>
The V2 Error group field only accepted header.GroupId, which would lead to validation errors when smpclient received v2 error responses from SMP groups beyond the standard set. This change allows creating ErrorV2 instances for both User Groups defined in the tree (i.e. Intercreate) as well as for User Groups defined outside of the tree.
Copy link
Owner

@JPHutchins JPHutchins left a comment

Choose a reason for hiding this comment

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

Thanks! Removed more lines than it added - ideal!

@JPHutchins JPHutchins merged commit e50868b into JPHutchins:main Feb 11, 2025
37 checks passed
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