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

Small error improvements #71

Merged
merged 5 commits into from
May 2, 2022
Merged

Small error improvements #71

merged 5 commits into from
May 2, 2022

Conversation

emersion
Copy link
Owner

@emersion
Copy link
Owner Author

Hm, "internal: return a DAVError instead of an Error" is a bit strange because it wraps DAVError into HTTPError, duplicating the HTTP code. We might want to pick between two strategies:

  • Use HTTPError with an Error wrapped in it to describe an XML DAV error. Remove DAVError.
  • Use DAVError to describe an XML DAV error, and use HTTPError for HTTP errors without an XML description. Never use Error as a Go error value.

@bitfehler
Copy link
Collaborator

Sorry, I never really looked much at the client code before, and it took me a moment to understand the error handling you added there 🙂

I am not entirely sure which approach sounds better. On the one hand, since both are essentially different protocols, separating them seems reasonable. Yet, since they map so closely to each other, maybe not. I think in the client library, both would be easy to implement, but I have no idea how tedious proper handling would be in a client application.

On the server side, however, I'd be worried that separating them would result in a lot of noisy code, like "if it's a http error and code X or a dav error and code X, do this, same for code Y, etc." without adding much value. If the only difference is a human-readable error message, getting that from an optional wrapped error seems much easier. Hence I'd slightly lean toward your first suggestion. But it's not a strong opinion and I might very well be missing something.

This allows callers to access the underlying error via errors.Unwrap.
That way we can avoid having different ways of representing the
same error value.
Same as NewOKResponse but for errors.
Instead of making the whole HTTP request fail when a single address
object cannot be fetched, return a partial error response.
@emersion
Copy link
Owner Author

emersion commented May 2, 2022

Here's how the first approach would look like.

Builds a detailed HTTPError + Error if the Response is a failure.
It contains more context than just the HTTPError.
@bitfehler
Copy link
Collaborator

Looks good to me... 🙂

@emersion emersion merged commit 3f8b212 into master May 2, 2022
@emersion
Copy link
Owner Author

emersion commented May 2, 2022

Thanks for the feedback!

@emersion emersion deleted the multiget-error-resp branch May 2, 2022 13:43
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.

None yet

2 participants