Closed aatifsyed closed 1 year ago
Last use of wrap
standing is
Which I think is fine, if not necessary because Error::invalid_size
is pub(crate)
Last use of
wrap
standing is
We should also change that to not generate code that is using deprecated functionality.
Do we have other const
functions that might be affected by the same problem? I wonder what a long-term solution could be here.
In the issue, you said that the problem is the io::Error
in Kind
? I tried previously to remove that one because it only appears in certain functions. Perhaps that is worth exploring because most functions don't actually return an io::Error
.
Last use of
wrap
standing isWe should also change that to not generate code that is using deprecated functionality.
This is not possible without further changes to Error
, which has no public constructors. I think that's outside the scope of this PR. But, to outline the problem,
Error
can only be created using methods like wrap
.
It must be created to implement MultihashDigest
.
Neither rustc -Dwarnings
nor clippy -Dclippy::pedantic
complain about the use of this method in derived code. I can add #[automatically_derived]
or #[allow(deprecated)]
if you would prefer.
As an aside, the docs for multihash-derive
are mangled
Do we have other
const
functions that might be affected by the same problem? I wonder what a long-term solution could be here.In the issue, you said that the problem is the
io::Error
inKind
? I tried previously to remove that one because it only appears in certain functions. Perhaps that is worth exploring because most functions don't actually return anio::Error
.
If you'd like my opinion, you should impl From<YourErrorType> for std::io::Error
, and have io functions return and io::Result
, which may be InvalidData
with a source = Some(YourErrorType)
, where YourErrorType
is a slimmer Error
(This would be a breaking change.) ((I think that's fine, you're pre 1.0.0, and the API should be allowed to evolve past current limitations))
I've gone ahead and implemented the required changes:
DigestTooBig
now contains a usize so it can match the error message from Error
. I think this is an odd choice - you're telling the caller the length of the slice, but they can already find that out because they retain slice ownership.From::from
) for Error
so that the derive macro doesn't use wrap
If you'd like my opinion, you should
impl From<YourErrorType> for std::io::Error
, and have io functions return andio::Result
, which may beInvalidData
with asource = Some(YourErrorType)
, whereYourErrorType
is a slimmerError
Good idea! Would you mind capturing this in an issue?
((I think that's fine, you're pre 1.0.0, and the API should be allowed to evolve past current limitations))
Yes-ish. multihash
is a foundational crate of the libp2p ecosystem. It appears pretty much in all public APIs because multiaddr
and PeerId
depend on it. A breaking change is a breaking change for every dependency. We just released a breaking change a few weeks ago and I'd like to not go through that effort again for a couple of months unless really necessary.
If you'd like my opinion, you should
impl From<YourErrorType> for std::io::Error
, and have io functions return andio::Result
, which may beInvalidData
with asource = Some(YourErrorType)
, whereYourErrorType
is a slimmerError
That sounds like a better solution. I would actually prefer that and keep the wrap
name and just doing a breaking change. I agree that breaking changes are a hassle, but it shouldn't prevent us from going for a clean API. As this is very specific, also project (like libp2p) don't need to upgrade, they can just keep using the current version as long as they are happy with it.
I'd also be OK with just having the proper solution merged into master and just keep it there before doing a release, in case more breaking changes are uncovered.
If you'd like my opinion, you should
impl From<YourErrorType> for std::io::Error
, and have io functions return andio::Result
, which may beInvalidData
with asource = Some(YourErrorType)
, whereYourErrorType
is a slimmerError
That sounds like a better solution. I would actually prefer that and keep the
wrap
name and just doing a breaking change. I agree that breaking changes are a hassle, but it shouldn't prevent us from going for a clean API. As this is very specific, also project (like libp2p) don't need to upgrade, they can just keep using the current version as long as they are happy with it.I'd also be OK with just having the proper solution merged into master and just keep it there before doing a release, in case more breaking changes are uncovered.
The issue is that you will have to immediately make a release because I guess that @aatifsyed would want to use this right away.
Not upgrading to latest solution is at best a band-aid and means what we might have to do more work later by backporting PRs instead of just being able to merge them into master.
An alternative suggestion could be to temporarily introduce a wrap_const
function, not deprecate wrap
and remove it again on the next breaking change.
Sorry for the week's silence, I was on vacation.
because I guess that @aatifsyed would want to use this right away.
You're correct, but as the rust-cid folks pointed-out^1, there's a useable workaround for Copy
types ^2:
const fn const_unwrap<T: Copy, E>(r: Result<T, E>) -> T {
let v = match r {
Ok(r) => r,
Err(_) => panic!(),
};
mem::forget(r);
v
}
That means forest features are no longer blocked - we can just use const_unwrap()
.
I think a lot of the discussion above about error types/API design is valuable, and maybe could be addressed in #332, but this PR (or something like it) can wait until after that.
I think a lot of the discussion above about error types/API design is valuable, and maybe could be addressed in #332, but this PR (or something like it) can wait until after that.
I agree with that. Thank you for the feedback and input on the error design. We'll incorporate it on the next breaking change :)
Resolves https://github.com/multiformats/rust-multihash/issues/330.