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

[WIP] Rust: Better errors #2976

Closed
wants to merge 4 commits into from

Conversation

Lukas-Dresel
Copy link
Contributor

I made an initial attempt at better error messages as discussed in the slack channel. I've implemented a custom BNError type we can use to designate various different forms of errors while keeping track of dependent Errors (causes) as well as the backtrace of each error.

This does unfortunately currently rely on the unstable backtrace feature:
#![feature(backtrace)]

For now, I only modified the code in binaryview.rs to use them, I'm happy to modify the rest after some review if this is something that will be incorporated :).

@fuzyll fuzyll requested a review from ElykDeer February 21, 2022 18:06
@ElykDeer ElykDeer changed the title Wip/better errors [WIP] Better errors Feb 21, 2022
@ElykDeer ElykDeer changed the title [WIP] Better errors [WIP] Rust: Better errors Feb 21, 2022
@ElykDeer ElykDeer marked this pull request as draft February 21, 2022 18:50
@Lukas-Dresel
Copy link
Contributor Author

Lukas-Dresel commented Feb 21, 2022

I did in the meantime push the code for all uses of Err in the API if i'm not mistaken, so it should be fully testable with all API logic. Unfortunately I only have a personal license, so I can't test any headless functionality (other than that I actually get a traceable error now when trying to open_view in headless :P)

@Lukas-Dresel
Copy link
Contributor Author

Lukas-Dresel commented Feb 21, 2022

On Slack, people noted that they disliked the naming of BNError and BNResult, I am more than happy to change this, it simply persisted while I was trying to grep for old uses of Result that I had missed.

For clarification, the comments were that there is no need for the BN prefix, and we should follow the std library model of just calling them Error and Result (like, e.g., std::io does).

I personally disagree, since I dislike having to lookup up which instance of Result it is whenever I see it, however, I cede the point that following the established convention might be better, and don't mind too much since I use an IDE :P

@ElykDeer
Copy link
Member

ElykDeer commented Jul 6, 2022

This is still something we want, and need to work towards resolving. No one is currently tasked on working on this, however, so we're closing this due to inactivity and will continue to track the need for this in an issue (#3245). If there's more work you want to do on this PR, feel free to reopen it.

@ElykDeer ElykDeer closed this Jul 6, 2022
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.

3 participants