Skip to content
This repository has been archived by the owner on Apr 29, 2024. It is now read-only.

Error handling improvements #35

Merged
merged 6 commits into from
Oct 11, 2022
Merged

Error handling improvements #35

merged 6 commits into from
Oct 11, 2022

Conversation

cloudhead
Copy link
Contributor

@cloudhead cloudhead commented Oct 10, 2022

Get rid of 99% of unwraps. Make unwraps warn by default, outside of test code.

See commits.

Closes #18

@cloudhead cloudhead marked this pull request as draft October 10, 2022 12:28
@cloudhead cloudhead force-pushed the cloudhead/error-handling branch 2 times, most recently from eedeccf to b66d80f Compare October 10, 2022 16:03
pub use ext::Error;
pub use ext::NotFound;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re: radicle-dev/radicle-git#29

Ah, I see this actually the blob re-exported. I don't think that should have been re-exported.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, how to handle that then?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are you using it for?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

@FintanH FintanH Oct 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd recommend you don't use it unless you're using that blob API in particular :)

Sorry for the confusion!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, the thing is it would be nice to have a "good" git error type, which git_ext::Error is pretty close to, or have maybe a RepositoryExt trait that is more flexible. The ref.target() case is a good example of this: most of the time you want the oid of a reference, and this involves repo.find_reference(...)?.target().ok_or(...)? which is cumbersome.

So I use my own methods for getting references, which returns a git_ext::Error, but this requires access to NotFound, so that I can return a "not found" error when the target is missing. Without this, I'd have to create yet another git error type..

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've created #38 to track this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case, you can use https://docs.rs/git2/0.15.0/git2/struct.Repository.html#method.refname_to_id, FYI :)

I'll continue the error discussion in the other issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Omg I didn't know about this! Life changing!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lol, I could swear I saw you use it before 😅

@cloudhead cloudhead merged commit a104f96 into master Oct 11, 2022
@cloudhead cloudhead deleted the cloudhead/error-handling branch October 11, 2022 13:51
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Audit code for unwrap use
2 participants