-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
basichost / blankhost: wrap errors #2331
Conversation
I’m ok with the error wrapping changes introduced here. I’d like to be a little bit more careful about introducing a new error type here. Not saying that it’s a bad idea, but I’d prefer to have an issue and an API discussion first. |
My toxic trait is coming up with contributions out of nowhere 😄. Ok, let's make an issue! |
Any chance you can take a look at this? |
I already reviewed this PR. Let me know once you've updated it. |
Ok, it wasn't clear whether changes were requested or not |
Updated to keep wrappings only |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... and so on for the other errors.
A user would usually wrap the error with "failed" or the log would also say that error, but I don't feel strongly about it, so applied. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Wondertan Any reason you only changed ~half of the error messages?
I simply applied all the comments |
Fixed now |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @Wondertan!
The goal here is to improve the experience of working with errors returned by the Host. With the current state, it's PITA to debug exactly what went wrong during a stream open and this feat attempts to solve it.
net
.errors.Join
Closes #2334