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

Add RPCError and Status #1656

Merged
merged 5 commits into from
Oct 2, 2023
Merged

Add RPCError and Status #1656

merged 5 commits into from
Oct 2, 2023

Conversation

glbrntt
Copy link
Collaborator

@glbrntt glbrntt commented Oct 2, 2023

Motivation:

One mistake we made in v1 was having the status also be an error. This leads to interesting situations where a status with code 'ok' can be treated as an error.

Here, we split them into two objects.

Modifications:

  • Add a new GRPCCore target
  • Add RPCError and Status types and tests

Result:

RPCError and status are separate types.

Motivation:

One mistake we made in v1 was having the status also be an error. This
leads to interesting situations where a status with code 'ok' can be
treated as an error.

Here, we split them into two objects.

Modifications:

- Add a new GRPCCore target
- Add RPCError and Status types and tests

Result:

RPCError and status are separate types.
@glbrntt glbrntt added the semver/none No version bump required. label Oct 2, 2023
/// An error representing the outcome of an RPC.
///
/// See also ``Status``.
public struct RPCError: @unchecked Sendable, Hashable, Error {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we add a comment why we use @unchecked here

}
}

/// Metadata associated with the error.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Couple of questions here:

  1. From the proposal I remember that this includes all metadata when I receive this error on the client side right? Can we document this?
  2. If I throw this error on the server side will all metadata be attached to the underlying transport response? Could we also document this?


/// A status object represents the outcome of an RPC.
///
/// Each ``Status`` is composed of a ``Status/code-swift.property`` and ``Status/message``. Each
Copy link
Collaborator

Choose a reason for hiding this comment

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

The first docc link is very interesting :D

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Status? 🤨

Copy link
Collaborator

Choose a reason for hiding this comment

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

I mean this Status/code-swift.property

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Off-by-one error ;)

Yeah, docc symbols aren't case sensitive so Code-the-type and code-the-property are ambiguous otherwise.

@glbrntt glbrntt enabled auto-merge (squash) October 2, 2023 15:10
@glbrntt glbrntt merged commit d576a74 into grpc:main Oct 2, 2023
@glbrntt glbrntt deleted the gb-rpc-error-and-status branch October 2, 2023 15:17
@glbrntt glbrntt added the v2 A change for v2 label Nov 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver/none No version bump required. v2 A change for v2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants