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

Support Error in no_std environments #268

Merged
merged 11 commits into from
Jul 5, 2023
Merged

Support Error in no_std environments #268

merged 11 commits into from
Jul 5, 2023

Conversation

JelteF
Copy link
Owner

@JelteF JelteF commented Jul 1, 2023

Resolves #261

Synopsis

The Error derive can be made to work well for the most part in
no_std environments by enabling #![feature(error_in_core)]. This
changes the Error derive slightly to import Error and related
traits from core, when the std feature is disabled.

In passing this also fixes actually running the nightly error tests. They were not actually run anymore because there was no build.rs file in the root of the repo, only in the impl package. So the nightly config was not available in tests.

Checklist

  • Documentation is updated (if required)
  • Tests are added/updated (if required)
  • CHANGELOG entry is added (if required)

The `Error` derive can be made to work well for the most part in
`no_std` environments by enabling `#![feature(error_in_core)]`. This
changes the `Error` derive slightly to import `Error` and related
traits from core, when the `std` feature is disabled.

Fixes #261
Copy link
Collaborator

@tyranron tyranron left a comment

Choose a reason for hiding this comment

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

There are some uncertainties and ideas to discuss.

@@ -18,7 +18,7 @@ pub fn expand(
let state = State::with_attr_params(
input,
trait_name,
quote! { ::std::error },
quote!{ ::derive_more::__private::Error },
Copy link
Collaborator

Choose a reason for hiding this comment

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

@JelteF how about just ::derive_more::Error?

We can bring the trait in scope together with the macro. This may be handy for much cases. And we can do this for all the traits (in a separate PR, surely).

Still, if someone needs only a macro, we could have done it like use derive_more::macro::Error.

Copy link
Owner Author

Choose a reason for hiding this comment

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

We can bring the trait in scope together with the macro. This may be handy for much cases. And we can do this for all the traits (in a separate PR, surely).

Yeah, I think that's probably a good idea. Because of this change I'm working on adding the following line to all our tests:

#![cfg_attr(not(feature = "std"), no_std)]

And automatically importing the traits, possibly from alloc would be quite useful.

Copy link
Owner Author

Choose a reason for hiding this comment

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

I'll leave it as __private here and we can make it public in the follow up PR where we do this for all traits.

impl/Cargo.toml Outdated Show resolved Hide resolved
tests/error/derives_for_enums_with_source.rs Show resolved Hide resolved
@@ -128,7 +132,7 @@ jobs:
go install github.com/pelletier/go-toml/cmd/tomljson@latest
echo "$HOME/go/bin" >> $GITHUB_PATH

- run: ci/test_all_features.sh
- run: ci/test_all_features.sh ${{ matrix.std }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

@JelteF I think here will be enough just to run tests 2 times sequentially, rather than matrix over the input parameter.

Copy link
Owner Author

Choose a reason for hiding this comment

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

I think it's quite nice to see if both no_std and std are broken with a quick glance at the job output. Also it would double our CI time, since it would be the longest running job by ~2x the previous one. So I changed this back to use a matrix. I did make the matrix a bit more descriptive though, so that the job output is clearer.

Copy link
Collaborator

@tyranron tyranron Jul 5, 2023

Choose a reason for hiding this comment

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

@JelteF you should edit branch protection rules, because now CI awaits test features job to complete, while we have test features (std) and test features (no_std) now only.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Done. If you approve the PR it should merge automatically.

@JelteF JelteF enabled auto-merge (squash) July 3, 2023 20:16
@JelteF JelteF merged commit d1ff0cf into master Jul 5, 2023
16 checks passed
@JelteF JelteF deleted the error-in-no_std branch July 5, 2023 11:35
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.

#[derive_more::Error] refers to std::
2 participants