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.

Sign up for GitHub

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

Jump to bottom

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

Closed
nox opened this issue May 21, 2024 · 5 comments · Fixed by #710
Closed

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

nox opened this issue May 21, 2024 · 5 comments · Fixed by #710

Comments

@nox
Copy link
Contributor

nox commented May 21, 2024

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
Copy link

vilgotf commented May 26, 2024

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
Copy link
Contributor Author

nox commented May 26, 2024

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.

nox added a commit to nox/bytes that referenced this issue May 26, 2024
@nox
Change Bytes::make_mut to impl From<Bytes> for BytesMut (closes tokio…
6b7368d 
…-rs#709)

<Arc<T>>::make_mut returns a &mut T, such an API is doable for Bytes too
and thus we should reserve Bytes::make_mut for that.

Furthermore, it would be helpful to use From<Bytes> as a trait bound
in some cases with other traits such as Hyper's body trait, where Hyper
gives you Bytes values.

Finally, making it impl From<Bytes> for BytesMut means the API is more
easily discoverable as it appears on both Bytes and BytesMut.
@vilgotf
Copy link

vilgotf commented May 26, 2024

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
Copy link
Contributor Author

nox commented May 26, 2024

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
Copy link
Contributor

This proposal seems reasonable to me.

@Darksonn Darksonn mentioned this issue May 28, 2024
Merged
Darksonn pushed a commit that referenced this issue May 28, 2024
@nox
Change Bytes::make_mut to impl From<Bytes> for BytesMut (closes #709) (…
fa1daac 
…#710)

<Arc<T>>::make_mut returns a &mut T, such an API is doable for Bytes too
and thus we should reserve Bytes::make_mut for that.

Furthermore, it would be helpful to use From<Bytes> as a trait bound
in some cases with other traits such as Hyper's body trait, where Hyper
gives you Bytes values.

Finally, making it impl From<Bytes> for BytesMut means the API is more
easily discoverable as it appears on both Bytes and BytesMut.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Change Bytes::make_mut to impl From<Bytes> for BytesMut nox/bytes
3 participants
@nox @Darksonn @vilgotf