Skip to content

Conversation

oguzkocer
Copy link
Contributor

This PR is a mockup for handling the API errors. It uses a top level enum WPApiError and then uses subtypes when appropriate, such as ClientErrorType. Endpoint specific errors would probably be handled this way as well, but it's hard to tell if that'd be the right setup without a full investigation of each endpoint.

One further idea might be to define suggested resolutions per error type - such as "Show a message to the user", "retry the request", "report the issue" etc. I am not sure if this library is the right place to define those resolutions, but it's something we should think about as that's the biggest consideration in terms of how much granular we'd like to go with our error types. If multiple errors are expected to be resolved in the same way, there is rarely a reason to separate each one.

With this setup, we should be able to handle generic errors from a centralized function and that should cover most our needs. We can also handle the parsing in a more generic way and plug the centralized error handling to it. That means, there is a good chance we can keep our endpoint specific functions clear and concise.


This PR also introduces NetworkResponseStatus which makes it easy to use http::StatusCode directly. One of the main disadvantages of having our network layer be swappable is that we can't directly work with the types from the libraries that we use. Defining our own traits and implementing them for the http crate could be the glue to make the wp_networking code cleaner.

I am not sure if this pattern is necessary for status code, because we could probably just use a u16 and that'd be enough. However, I wanted to keep this implementation for now as a reference to this pattern.

@oguzkocer oguzkocer requested a review from jkmassel January 17, 2024 18:30
@oguzkocer oguzkocer merged commit 46eac93 into trunk Jan 21, 2024
@oguzkocer oguzkocer deleted the error-handling-prototype branch January 21, 2024 02:58
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.

1 participant