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 std::error::Error for cocoon::Error #26

Closed
wants to merge 7 commits into from

Conversation

madonuko
Copy link
Contributor

This adds the std::error::Error trait implementation to cocoon::Error using the thiserror crate.
Note that this is only limited to the presence of feature "std".

This adds the `std::error::Error` trait implementation to
`cocoon::Error` using the `thiserror` crate.
Note that this is only limited to the presence of feature "std".
Copy link
Owner

@fadeevab fadeevab left a comment

Choose a reason for hiding this comment

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

Thanks for the PR

src/error.rs Outdated Show resolved Hide resolved
src/error.rs Outdated Show resolved Hide resolved
@madonuko
Copy link
Contributor Author

Might be good to update the nightly version in CI

@fadeevab
Copy link
Owner

Might be good to update the nightly version in CI

There is a weird problem with coverage, the coverage works well on that specific nightly version, and it may take a lot of time to fix coverage in case of shifting to another version...

Could you change your patch to a classic variant without "dep:"?

@madonuko
Copy link
Contributor Author

madonuko commented May 23, 2024

I've further tested this on my computer and the best I could do is to remove line 36 entirely, but that'd create a very awkward case where people can enable the thiserror feature without the std feature

So maybe we could use -Z namespaced-features instead in CI?

@fadeevab
Copy link
Owner

@madonuko I think it's okay to remove line 36, and maybe someone would use it without std :)

BTW, Error derives Debug: do you find it insufficient and do you prefer simple Display type of errors? Or you specifically need an Error trait implementation?

@madonuko
Copy link
Contributor Author

I just kind of thought it'd be beneficial to implement Display for Error so that you get better diagnostics when your program panics…

@fadeevab
Copy link
Owner

@madonuko I'm looking whether I can bump the nightly version for coverall...

Cargo.toml Outdated
@@ -31,7 +32,7 @@ default = ["std"]

# Enables all features, including support of simplified Cocoon API, using `rand::thread_rng`,
# and API related to `std::io`: wrap to writer, unwrap from reader.
std = ["alloc", "rand/std"]
std = ["alloc", "rand/std", "thiserror"]
Copy link
Owner

@fadeevab fadeevab May 26, 2024

Choose a reason for hiding this comment

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

@madonuko Let's make it optional, you will enable it in your code.

Suggested change
std = ["alloc", "rand/std", "thiserror"]
std = ["alloc", "rand/std"]

Then I can quickly merge it, I will improve the documentation and I will release it shortly.

Copy link
Owner

Choose a reason for hiding this comment

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

@madonuko Broke your PR... See the fix: #28

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks pretty much perfect to me

@fadeevab
Copy link
Owner

Closed in favor of PR #28 which includes the contents of this PR.

@fadeevab fadeevab closed this May 27, 2024
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