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

adds anyhow where result used to be #860

Merged
merged 6 commits into from
Mar 3, 2022
Merged

Conversation

eureka-cpu
Copy link
Contributor

Intended to close #851

I've gone through and replaced any instance I could find of Result<(), String> with anyhow, along with error messages.

Copy link
Member

@JoshuaBatty JoshuaBatty left a comment

Choose a reason for hiding this comment

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

Nice, looks good thanks. Just left some comments to help you get the tests working again.

There does still remain the question if we should be considering using thiserror instead of anyhow if we are exposing forc as a library. But this is still an upgrade from the current error handling and can be addressed in a separate PR if we decide to move to thiserror.

forc/src/utils/restricted_names.rs Show resolved Hide resolved
forc/src/utils/restricted_names.rs Show resolved Hide resolved
forc/src/utils/restricted_names.rs Show resolved Hide resolved
forc/src/utils/restricted_names.rs Show resolved Hide resolved
forc/src/utils/restricted_names.rs Outdated Show resolved Hide resolved
@mitchmindtree
Copy link
Contributor

Ahh was hoping to have this addressed after landing #825 but I should have mentioned so in the issue 🥲 if this happens to land first I can rebase on top no worries 👍


I would like to see thiserror with more granular types eventually, but I think separating the parts of forc that we wish to expose as a library more clearly first might help to go about this, I'll open an issue with some thoughts on this. For now though I think anyhow and this PR makes a lot of sense!

@eureka-cpu eureka-cpu requested a review from JoshuaBatty March 3, 2022 00:14
@JoshuaBatty
Copy link
Member

I think it probably looked tidier the way you first had it, eg:

assert_eq!(
    contains_invalid_char("test#proj", "package name").map_err(|e| e.to_string()),
    std::result::Result::Err(
        "invalid character `#` in package name: `test#proj`, \
            characters must be Unicode XID characters \
            (numbers, `-`, `_`, or most letters)"
            .into()
    )
);

Copy link
Member

@JoshuaBatty JoshuaBatty left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for addressing those changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Make error handling more consistent throughout forc
3 participants