remove void dep, use core::convert::Infallible instead
#482
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rursprung
force-pushed
the
remove-void-dependency
branch
from
9f2a3f9 to
fc90c0a
Compare
Rahix
reviewed
Jan 11, 2024
There was a problem hiding this comment.
Yeah, it's time to drop void. However in my eyes it isn't quite as simple as this. I am really not a fan of switching to regular .unwrap() in all the example code. Please check rust-embedded/embedded-hal#329 for a detailed discussion of the reasons why. This linked issue also has the solution I'd prefer to use in our examples: The unwrap-infallible crate. I think we should put it into the avr-hal prelude as the direct replacement for ResultVoidExt.
there's no need for the `void` dependency anymore. accordingly it can be removed. this will also help with implementing `embedded-hal` v1 traits as some of them rely on (or at least have better support for) `core::convert::Infallible`. this is a breaking change for consumers as they have to e.g. replace `void_unwrap()` calls with `unwrap()`.
After removing `void`, we're still missing a way to explicitly unwrap infallible results such that it is clear that no panic can occur in this location. Because the standard library does not yet provide a solution to this, let's use the `unwrap-infallible` crate [1] in the meantime. This topic was also discussed in more detail in the embedded-hal project [2]. [1]: https://crates.io/crates/unwrap-infallible [2]: rust-embedded/embedded-hal#329
Rahix
force-pushed
the
remove-void-dependency
branch
from
fc90c0a to
4f38d8c
Compare
|
I took the liberty to add the unwrap_infallible here so it's part of the same PR :) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
there's no need for the
voiddependency anymore. accordingly it can be removed. this will also help with implementingembedded-halv1 traits as some of them rely on (or at least have better support for)core::convert::Infallible.this is a breaking change for consumers as they have to e.g. replace
void_unwrap()calls withunwrap().the prelude is now no longer needed in most cases (potentally the whole
mod preludecould be removed in the future as it now adds little benefit?).note that this PR will conflict with #481; when one gets merged i'll have to rebase the other on top of it (though it will be a fairly trivial rebase). this is due to the fact that both modify the same lines in a few places. merging this one here first might be easier as the other one is smaller.