search

tokio-rs / bytes

Utilities for working with bytes
MIT License
1.86k stars 278 forks source link

Consider replacing Bytes::make_mut by impl From<Bytes> for BytesMut #709

Closed nox closed 3 months ago

nox commented 3 months ago

Bytes::make_mut is a very good addition to the API but I think it would be better if it was instead exposed as <BytesMut as From<Bytes>>::from. Could this be done before the next bytes version is released? Bytes::make_mut isn't released yet.

vilgotf commented 3 months ago

The reason for the current API is to support adding a fallible Bytes::try_mut method in the future (as I proposed in #611). See also #368

nox commented 3 months ago

None of this mentions From<_> though. For some HTTP stuff I need to mutate bytes yielded by Hyper and I can do that in-place, but Hyper yields Bytes so I want to be able to do O: Into<BytesMut>.

Note also that this try_mut you suggest is what should be called make_mut to mimic Arc<_>, so that's one more argument in favour of making the current Bytes::make_mut become an impl From<Bytes> for BytesMut.

vilgotf commented 3 months ago

Oh yes, I had forgotten that I wanted make_mut to be the fallible conversion method. With that change I'm no longer against this (I was opposed to this from a naming standpoint, I felt that the try_mut and from names would conflict and lead to confusion).

nox commented 3 months ago

Oh yes, I had forgotten that I wanted make_mut to be the fallible conversion method. With that change I'm no longer against this (I was opposed to this from a naming standpoint, I felt that the try_mut and from names would conflict and lead to confusion).

For the API you envision, following what Arc<T> does, you should add Bytes::make_mut(&mut self) -> &mut T and Bytes::get_mut(&mut self) -> Option<&mut T>.

Darksonn commented 3 months ago

This proposal seems reasonable to me.