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

Implement anyhow and thiserror #90

Merged
merged 2 commits into from Sep 22, 2022
Merged

Implement anyhow and thiserror #90

merged 2 commits into from Sep 22, 2022

Conversation

sophielouisefeith
Copy link
Contributor

@sophielouisefeith sophielouisefeith commented Sep 20, 2022

Changed

• Change error handling to use the anyhow crate in bin
• Change error handling to use the thiserror crate in lib

Resolves #87

@sophielouisefeith sophielouisefeith force-pushed the this_anyhow_error branch 2 times, most recently from 243da1d to aa02975 Compare September 21, 2022 17:47
@sophielouisefeith sophielouisefeith changed the title Add thiserror to lib Improve error handeling Sep 21, 2022
@sophielouisefeith sophielouisefeith marked this pull request as ready for review September 21, 2022 17:52
src/bin/interactive.rs Outdated Show resolved Hide resolved
Copy link
Member

@herr-seppia herr-seppia left a comment

Choose a reason for hiding this comment

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

LGTM overall

However the CHANGELOG.md update is required before merge

In addition I have a doubt about the wrong password handling
The rest are minor nits and leftover

src/bin/interactive.rs Outdated Show resolved Hide resolved
src/bin/interactive.rs Outdated Show resolved Hide resolved
src/bin/interactive.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/error.rs Show resolved Hide resolved
src/error.rs Outdated Show resolved Hide resolved
src/bin/main.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@ZER0 ZER0 left a comment

Choose a reason for hiding this comment

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

LGTM but few comments needs to be addressed.
Also in general I would have preferred have two PRs, one solely for the Error Handling, but it's okay for this time.
However, you need to have at least two separate commits for that.

The commit messages should looks like

Implement `anyhow` and `thiserror`

- Change error handling to use the `anyhow` crate in `bin`
- Change error handling to use the `thiserror` crate in `lib`

Resolves #87

And the other:

Improve UX on password's retrying scenarios

- Change program behavior to quit if wrong seed phrase is given [#49] 
- Change program behavior to have three attempts for entering a password [#46]

Resolves #46, #49

src/bin/interactive.rs Outdated Show resolved Hide resolved
src/error.rs Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/bin/io/prompt.rs Outdated Show resolved Hide resolved
@sophielouisefeith sophielouisefeith force-pushed the this_anyhow_error branch 2 times, most recently from 5155526 to 51f9bd7 Compare September 22, 2022 09:57
@sophielouisefeith sophielouisefeith changed the title Improve error handeling Implement anyhow and thiserror Sep 22, 2022
Copy link
Member

@herr-seppia herr-seppia left a comment

Choose a reason for hiding this comment

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

LGTM

src/error.rs Outdated Show resolved Hide resolved
• Change error handling to use the `anyhow` crate in `bin`
• Change error handling to use the `thiserror` crate in `lib`

Resolves #87
Copy link
Contributor

@ZER0 ZER0 left a comment

Choose a reason for hiding this comment

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

LGTM

@sophielouisefeith sophielouisefeith merged commit 9d90912 into main Sep 22, 2022
@sophielouisefeith sophielouisefeith deleted the this_anyhow_error branch September 22, 2022 10:56
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.

Implement anyhow and thiserror
4 participants