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 core::error::Error for ParsingError #28

Merged
merged 1 commit into from
Jan 11, 2023
Merged

Implement core::error::Error for ParsingError #28

merged 1 commit into from
Jan 11, 2023

Conversation

jlxip
Copy link
Contributor

@jlxip jlxip commented Jan 11, 2023

This continues the discussion in #27.

Regarding cole14's last comment, this does work in my use case. However, implementing the trait core::error::Error instead of std::error::Error appears to be an experimental feature, which can only be used with nightly Rust.

For this reason, I've taken the liberty to add a new optional and non-default feature to the crate, nightly, since apparently it's not possible to deduce the release channel at compile time. Now:

  • If the std feature is enabled, the original code is compiled.
  • If the std feature is not enabled and the nightly feature is enabled, the experimental feature #![feature(error_in_core)] is used, and a similar† branch of the original code is compiled, one that substitutes std with core and does not match ParseError::IOError, which is only available with std.

† Maybe there's a better way to do this? I haven't looked extensively at the Rust preprocessor. Is there a #define-like capability?

@cole14
Copy link
Owner

cole14 commented Jan 11, 2023

Awesome, thanks for the background link to the progress towards Error being available from libcore on nightly. Looks like that got merged in for 1.65.0 which landed a few weeks after I added the no_std support to this crate. :)

I think the approach here looks fine to me and the duplicated code for different features seems to be a common pattern for these sorts of things.

@jlxip
Copy link
Contributor Author

jlxip commented Jan 11, 2023

Cool. Merge whenever you're ready then. Thank you :)

@cole14 cole14 merged commit bddf58a into cole14:master Jan 11, 2023
@cole14
Copy link
Owner

cole14 commented Jan 12, 2023

Thanks again for the report and for the PR!

I want to say here in case you're wondering, that I'm going to wait a few weeks before I bundle this into an new crate version just in case further changes come in after this. My hope is to try and avoid churning crate versions too quickly. It sounds like that's probably fine for your use-case, but if you'd like me to push an updated crate with these changes sooner than that let me know.

@jlxip
Copy link
Contributor Author

jlxip commented Jan 12, 2023

Oh, it's fine. Thanks a lot for the offer though. I'm using the git version and it works like a charm :)

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