Skip to content

[WIP] Replace unwrap calls in asyncgit with error handling - closes #53 #58

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

Merged
merged 26 commits into from
May 15, 2020

Conversation

MCord
Copy link
Contributor

@MCord MCord commented May 14, 2020

This pull request tries to replace unwrap() calls in asyncgit with error handling via a custom error type defined in error.rs. changes has been split into separate commits.

  • Added a new error type in error.rs file (dependent on thiserror crate)
  • Added a Result<T> type alias for Result<T,Error>
  • Removed all unwrap() calls from asyncgit lib except a few in callbacks that I don't know how to deal with for the moment.

I have not touched unwrap() inside tests as I am not sure yet if we want to remove them too. I assume they are alright in tests. I will continue to change other files when I get feedback about the code.

@MCord MCord changed the title [WIP] Replace unwrap calls in asyncgit with error handling [WIP] Replace unwrap calls in asyncgit with error handling - Closes #53 May 14, 2020
@MCord MCord changed the title [WIP] Replace unwrap calls in asyncgit with error handling - Closes #53 [WIP] Replace unwrap calls in asyncgit with error handling - closes #53 May 14, 2020
@MCord MCord force-pushed the issue_53_more_error_handling branch from 38ab97d to 3e4d1e9 Compare May 14, 2020 13:06
@MCord MCord mentioned this pull request May 14, 2020
@extrawurst
Copy link
Collaborator

Hey @MCord thanks for getting at this!
Let me annotate some questions that I have. Please keep in mind as you see in the codebase, I am also no expert on error handling in Rust yet :)

removed an obsolete comment.
@extrawurst
Copy link
Collaborator

extrawurst commented May 14, 2020

Really good work!

Two more things:

  1. Using unwrap in tests is fine 👍
  2. Will you convert sync::utils aswell? then we have repo returning Result which is used everywhere and also allow the unwrap for repo.workdir (see other conversation)

@MCord
Copy link
Contributor Author

MCord commented May 14, 2020

Thanks ❤️ !

I will remove every unwrap outside the tests and update the PR. I don't mind changing sync::utils everywhere since it gives me chance to read more of the project.

@extrawurst
Copy link
Collaborator

Sorry to be such a pain in the ass 🙈! I am super glad you do all of this!

@MCord
Copy link
Contributor Author

MCord commented May 15, 2020

Sorry to be such a pain in the ass 🙈! I am super glad you do all of this!

My pleasure! sorry you have to micro mange this PR :) It's my first lets hope It gets better with time 😂

@extrawurst
Copy link
Collaborator

Looks good to me! I will squash merge it if that's ok by you?

@MCord
Copy link
Contributor Author

MCord commented May 15, 2020 via email

@extrawurst extrawurst merged commit 03505b9 into gitui-org:master May 15, 2020
@extrawurst
Copy link
Collaborator

Thank you @MCord! Great job!👍

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