Skip to content

Conversation

@glbrntt
Copy link
Collaborator

@glbrntt glbrntt commented Jun 11, 2020

Motivation:

GRPCStatus has a static .ok status which is useful when implementing
service providers. However, since the status also includes a message it's
too easy for someone receiving a status to check status == .ok where
they really mean 'status.code == .ok'.

Modifications:

  • Add a convenience .isOk to GRPCStatus
  • Add a note to .ok

Result:

Motivation:

`GRPCStatus` has a static `.ok` status which is useful when implementing
service providers. However, since the status also includes a message it's
too easy for someone receiving a status to check `status == .ok` where
they really mean 'status.code == .ok'.

Modifications:

- Add a convenience `.isOk` to `GRPCStatus`
- Add a note to `.ok`

Result:

- Slightly reduced chance of user error.
- Resolves grpc#821
@glbrntt glbrntt added nio 🆕 semver/minor Adds new public API. labels Jun 11, 2020
@glbrntt glbrntt requested a review from Lukasa June 11, 2020 09:55
Copy link
Collaborator

@Lukasa Lukasa left a comment

Choose a reason for hiding this comment

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

WFM, I think this is something @MrMage also cared about?

@MrMage
Copy link
Collaborator

MrMage commented Jun 11, 2020

LGTM; we could still consider to make .ok internal, which is a bit more controversial.

@glbrntt glbrntt merged commit b648172 into grpc:master Jun 12, 2020
@glbrntt
Copy link
Collaborator Author

glbrntt commented Jun 12, 2020

I think this is probably sufficient.

@glbrntt glbrntt deleted the gb-status-isok branch June 12, 2020 07:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🆕 semver/minor Adds new public API.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Consider internalising the convenience "commonly used" statuses

3 participants