Skip to content

Conversation

@st31ny
Copy link
Collaborator

@st31ny st31ny commented Oct 27, 2019

I did some refactoring of the error handling mechanisms and tried to address the issues #71 and #72.
I introduced a new ApiError for custom errors with message and optional code and data. The usage of KeyError for method-not-found and TypeError for invalid params have been replaced by custom exceptions, so that there is no confusion between these client errors and other internal server errors.

A point for discussion is whether to keep AssertionErrors causing an InvalidParamsResponse. While assertions can be used for argument validation they might also simply check internal server invariants which should cause a generic server error.

Note: I only tested against py37. Can sb run the tests on 3.6 and 3.8, please?

* Allows methods to signal user-defined error condition.
* Avoid responding with a method not found error if a KeyError is raised
  in a method implementation.
* Avoid confusion with random type errors in methods.
@bcb
Copy link
Member

bcb commented Oct 27, 2019

Do you mind splitting this into two PRs, one for #71 and one for #72?

The one for #72 should target the 5.0 branch since it’s a breaking change. #71 can go to master.

@st31ny
Copy link
Collaborator Author

st31ny commented Oct 28, 2019

Sure, no worries. So, I guess the InvalidArgumentsError should target the 5.0 branch, too?

@st31ny st31ny closed this Oct 28, 2019
@st31ny st31ny deleted the error_refactoring branch October 28, 2019 16:54
@bcb
Copy link
Member

bcb commented Oct 28, 2019

It’s not breaking so it can go into a minor release, as long as we leave AssertionError for now.

@st31ny
Copy link
Collaborator Author

st31ny commented Oct 29, 2019

Ok, sure, I don't mind getting this into a minor. I have opened #94 and #95, so we can merge these things separately (or together).

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