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

refactor(error): improve error handling #252

Merged
merged 3 commits into from
Dec 2, 2022
Merged

Conversation

dan-da
Copy link

@dan-da dan-da commented Nov 27, 2022

Addresses issue #247. (part a: more to come later)

I started small by just fixing 4 unwrap() calls in kindelia_core/src/net.rs.

Unfortunately one of these involved a trait, which complicated things a bit and required small changes in other files.

In kindelia main.rs I could have just mapped the errors from net.rs into a string and submitted a smaller PR, but instead I decided it would be cleaner to handle them as anyhow::Result.

I ended up changing most fn in main.rs and config.rs to return anyhow::Result instead of Result<_, String>. This required some calls to map_err(|e| anyhow!(e) to deal with String errors from format!() or from lower level fn calls. However, as the lower level calls get fixed, then the map_err goes away and we will just have fn_call()?. very clean.

note: the map_err() is necessary because anyhow::Result accepts anyhow::Error which is a boxed dynamic error type that accepts any error type derived from std::error::Error. However String is not so derived and thus must be mapped (or preferably return anyhow::Result or a thiserror type instead).

So this is very much a preliminary / intermediate PR to introduce anyhow and show the direction.

I understand there is a big refactor happening in another branch. I am ok with rebasing onto it when merged. Or this could be merged first, whichever works.


kindelia_core:

  • removes all unwrap() from net.rs
  • defines error enums in net.rs: ProtoCommError and ParseAddressError
  • return Result from affected fn in net.rs
  • change IPV6 panic!() to unimplemented!()
  • accept addr arg for Node::new() so it can avoid returning Result
  • update test and bench as necessary

kindelia:

  • add dep on anyhow crate
  • return anyhow::Result from config methods
  • return anyhow::Result from main methods, including main()
  • return anyhow::Result from FileInput::read_to_string()

@dan-da
Copy link
Author

dan-da commented Nov 27, 2022

Something seems wonky with CI. afaict, checks are only running when the PR is first created, and not for subsequent push(es). or at least it does not run for some subsequent pushes. is this intentional?

@dan-da dan-da force-pushed the issue_247_a branch 2 times, most recently from f6e05cf to 2456a70 Compare November 28, 2022 06:46
@dan-da
Copy link
Author

dan-da commented Nov 28, 2022

ok, so I was getting some emails from github that CI was failing. Here's an example:
https://github.com/dan-da/Kindelia/actions/runs/3562238811

It was some kind of cargo bench issue. I found a Cargo.toml workaround, and pushed changes for it to this branch. After which, I get another kind of failure:
https://github.com/dan-da/Kindelia/actions/runs/3562393608

Run racs4/github-action-benchmark@v1
/usr/bin/git -c user.name=github-action-benchmark -c [email protected] -c http.https://github.com/.extraheader= fetch ***github.com/dan-da/Kindelia.git gh-pages:gh-pages
fatal: couldn't find remote ref gh-pages
Error: Command 'git' failed with args '-c user.name=github-action-benchmark -c [email protected] -c http.https://github.com/.extraheader= fetch ***github.com/dan-da/Kindelia.git gh-pages:gh-pages': fatal: couldn't find remote ref gh-pages
: Error: The process '/usr/bin/git' failed with exit code 128

maybe it wants me to have a gh-pages branch for some reason?

Addresses issue kindelia#247.  (more to come later)

kindelia_core:

* removes all unwrap() from net.rs
* defines error enums in net.rs: ProtoCommError and ParseAddressError
* return Result from affected fn in net.rs
* change IPV6 panic!() to unimplemented!()
* accept addr arg for Node::new() so it can avoid returning Result
* update test and bench as necessary

kindelia:

* add dep on anyhow crate
* return anyhow::Result from config methods
* return anyhow::Result from main methods, including main()
* return anyhow::Result from FileInput::read_to_string()
Fixes calls to anyhow!() macro of these two forms:

* anyhow!(format(x,y))        --> anyhow!(x,y)
* anyhow!("...".to_string())  --> anyhow!("...")
@racs4
Copy link
Contributor

racs4 commented Dec 2, 2022

Something seems wonky with CI

I think this is solved in ab32905, but you can talk if you have any problems.

then the map_err goes away and we will just have fn_call()?. very clean

This seems very good.

Or this could be merged first

I think this is better, since this is smaller.

kindelia_core/src/node.rs Outdated Show resolved Hide resolved
Addresses kindelia#247.

use anyhow::Context trait to Replace calls to
map_err(|e| anyhow!("...{}", e)) with .context("...").

This does two things:
1. Makes the code and error messages more succinct and readable.
2. Preserves the original error
@dan-da
Copy link
Author

dan-da commented Dec 2, 2022

I just force pushed with these changes:

  1. removed Cargo.toml bench sections to disable benches
  2. added a comment to Node::new() as requested

@dan-da dan-da requested a review from racs4 December 2, 2022 07:01
@racs4 racs4 merged commit 5b5e917 into kindelia:dev Dec 2, 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.

2 participants