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

Add error source implementation #825

Merged
merged 2 commits into from
Apr 10, 2021

Conversation

viraptor
Copy link
Contributor

Without this it's hard to figure out in code what's the real reason for
the failure. Parsing the description is not enough.

Without this it's hard to figure out in code what's the real reason for
the failure. Parsing the description is not enough.
@BratSinot
Copy link

Any progress? I need such functionality to know about websocket connection errors.

@danii
Copy link

danii commented Apr 8, 2021

I personally feel that the current Error type in warp is fundamentally flawed. It shouldn't store a dynamic reference to the error, it should be able to store the error inside itself and give you information from it, such as an OS error code or any data pertaining to the error. Extending the current implementation just makes it harder to switch to an implementation like that. That's just my two cents though.

@viraptor
Copy link
Contributor Author

viraptor commented Apr 9, 2021

@danii I don't believe you can realistically wrap all possible exceptions in this case and forward their properties. They could be segregated into some groups which could give back enough details that you don't have to dig into source in a common case. But writing a mega-wrapper which can handle every case of ssl, socket, parsing, logic, etc. error and keeping it complete over time is unlikely to be realistic. (if one of the dependencies get updated and includes a new possible error case, warp apps couldn't examine that until warp is updated (and that would bump minimum required dep version to support that error))

Also, some of those underlying errors already implement .source(), so warp would have to both forward those and not implement its own .source(), which is weird.

.source() is in the standard library already - deviating from that should have a really strong reason.

@jxs jxs merged commit 4a03710 into seanmonstar:master Apr 10, 2021
@jxs
Copy link
Collaborator

jxs commented Apr 10, 2021

LGMT thanks!

@viraptor viraptor deleted the viraptor/error-source branch April 10, 2021 23:19
jxs pushed a commit to jxs/warp that referenced this pull request Jun 27, 2021
* Add error source implementation
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

4 participants