Skip to content

RSDK-458 add server error interceptor #254

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 11 commits into from
Apr 17, 2023
Merged

RSDK-458 add server error interceptor #254

merged 11 commits into from
Apr 17, 2023

Conversation

purplenicole730
Copy link
Member

No description provided.

@purplenicole730 purplenicole730 marked this pull request as ready for review April 11, 2023 22:35
@purplenicole730 purplenicole730 requested a review from a team as a code owner April 11, 2023 22:35
zaporter-work
zaporter-work approved these changes Apr 12, 2023
View reviewed changes
Copy link

@zaporter-work zaporter-work left a comment

Choose a reason for hiding this comment

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

LGTM % nits.

@cheukt cheukt requested a review from njooma April 12, 2023 14:47
@purplenicole730 purplenicole730 removed the request for review from benjirewis April 12, 2023 15:40
benjirewis
benjirewis reviewed Apr 12, 2023
View reviewed changes
Copy link
Member

@benjirewis benjirewis left a comment

Choose a reason for hiding this comment

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

How does this change interact with the various calls to grpc_error (e.g. here). Will we be "double" wrapping errors?

cheukt
cheukt requested changes Apr 12, 2023
View reviewed changes
Copy link
Member

@cheukt cheukt left a comment

Choose a reason for hiding this comment

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

looks great! would like to see some tests and a pass at better var names

njooma
njooma requested changes Apr 12, 2023
View reviewed changes
njooma
njooma approved these changes Apr 13, 2023
View reviewed changes
Copy link
Member

@njooma njooma left a comment

Choose a reason for hiding this comment

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

nice

cheukt
cheukt requested changes Apr 13, 2023
View reviewed changes
Copy link
Member

@cheukt cheukt left a comment

Choose a reason for hiding this comment

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

would like test the behavior of the code a bit more, but otherwise looks great!

cheukt
cheukt reviewed Apr 13, 2023
View reviewed changes
@purplenicole730 purplenicole730 requested a review from cheukt April 14, 2023 18:51
cheukt
cheukt approved these changes Apr 14, 2023
View reviewed changes
Copy link
Member

@cheukt cheukt left a comment

Choose a reason for hiding this comment

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

LGTM - just have a request for the test

@purplenicole730 purplenicole730 merged commit b57f2f7 into viamrobotics:main Apr 17, 2023
@purplenicole730 purplenicole730 deleted the RSDK-458-server-error-interceptor branch April 17, 2023 18:02
njooma pushed a commit to njooma/viam-python-sdk that referenced this pull request Apr 18, 2023
@purplenicole730 @njooma
RSDK-458 add server error interceptor (viamrobotics#254)
1fecc45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@benjirewis benjirewis benjirewis left review comments

@zaporter-work zaporter-work zaporter-work approved these changes

@njooma njooma njooma approved these changes

@cheukt cheukt cheukt approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@purplenicole730 @njooma @benjirewis @cheukt @zaporter-work