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 jsonrpc response None result missing #1

Closed
wants to merge 1 commit into from

Conversation

kingosticks
Copy link

@kingosticks kingosticks commented Mar 11, 2024

I appear to have left all the tests broken

@kingosticks
Copy link
Author

Before I fix the tests, do you want this or do you want to take it in another direction?

@jodal
Copy link
Owner

jodal commented Mar 12, 2024

Fixing the tests is probably just about replacing Response with the right one of the two alternatives. However, if we do .decode(..., type=Response) anywhere in the implementation, that will not work without adding some tag field to the types (which is not in our power to change) to be able to differentiate them.

I think an alternative approach is to change the Response class to use another default value, so that the right fields are omitted in the serialization of each case:

class Response(msgspec.Struct, kw_only=True, omit_defaults=True):
    jsonrpc: Literal["2.0"]
    id: RequestId | None  # None is allowed, but it must be set explicitly.
    result: Any | None | msgspec.UNSET = msgspec.UNSET
    error: ErrorResponseDetails | msgspec.UNSET = msgspec.UNSET

    @classmethod
    def as_success(cls, id: RequestId, result: Any | None) -> Self:
        return cls(jsonrpc="2.0", id=id, result=result)

    @classmethod
    def as_error(cls, id: RequestId | None, error: ErrorResponseDetails) -> Self:
        return cls(jsonrpc="2.0", id=id, error=error)

@kingosticks
Copy link
Author

I was struggling with what to use as an alternative default, msgspec.UNSET is much neater. I prefer your fix.

@jodal
Copy link
Owner

jodal commented Mar 12, 2024

Implemented in 0fde81e.

@jodal jodal closed this Mar 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants