Consider renaming Buf::bytes() and BufMut::bytes_mut()

[carllerche]

These two functions are named in a way that implies they return the entire set of bytes represented by the Buf/BufMut.

Some options:

• window() / window_mut()
• chunk() / chunk_mut().
• fragment() / fragment_mut()

[carllerche]

@tokio-rs/maintainers Anyone have thoughts? If there isn't any support for a change here, I am inclined to just let it be.

[Ralith]

I agree that the current name could be misleading, and my knee-jerk reaction to window is positive--it's at least ambiguous enough to prompt careful examination.

[Darksonn]

I support changing it to chunk. This is consistent with the naming in std on the slice::windows and slice::chunks methods since our chunks are not overlapping.

[rcoh]

As a data point, this personally bit me because I didn't read the docs. It doesn't help that if you have a Chain, I don't think you see the doc on bytes easily from an IDE. I certainly expected .bytes() with no context specified to "magically" give me all the bytes.

Maybe next_bytes()? although that perhaps carries the unwanted connotation that it will advance it

[dekellum]

NP-hard Question: Is the sum of time saved for new users (by reduced confusion) strictly greater than (>) the one time cost of all downstream existing users changing call names?

[rcoh]

I would bet that there are downstream users that will discover buggy code when the name changes...

[dekellum]

Can we deprecate aliases of old names, to amortize the cost for downstream?

[Darksonn]

We are not going to publish bytes v1.0.0 containing a deprecated method. That's just silly.

[hawkw]

We are not going to publish bytes v1.0.0 containing a deprecated method. That's just silly.

I think the idea is that we would publish a point version of 0.6 that adds the new names as aliases to the old ones, and deprecates the old ones, as an "advance warning" to users prior to upgrading to 1.0?

[Darksonn]

That's fine with me.

[dekellum]

Yeah, like in the same way that following Semver is silly? My idea was/is to include it deprecated in 1.0, silly be damned. The confusion avoidance is still achieved that way, possibly with even more context to show that the name has changed.

Edit: And your idea, @hawkw, is compatible with mine, but I don't know if that is worth the trouble.

[carllerche]

@rcoh I did consider next_bytes() some. I am worried about the association with Iterator::next which implies that calling the fn implies advancing the cursor.

[rcoh]

peek_bytes()?

[Darksonn]

In that case, what about peek_chunk()?

[carllerche]

why not just chunk() and chunk_mut()?

[dekellum]

I would bet that there are downstream users that will discover buggy code when the name changes...

Maybe, but for every one of those, there will be two more silly users that will just blindly search and replace rename the method calls. That's certainly my plan! 🥇

Similar to the busy work inflicted by #433, in practical terms.

2¢

[seanmonstar]

We can't save everyone. But, if the changelog entry about the rename mentions "this change was motivated by noticing users frequently misunderstanding how it works", that might convince some of those to consider if they should analyze each place they used it.

[dekellum]

But if there was a deprecation warning on compile, that warning could say the same, and that might be more noticeable to the more silly users?

[dekellum]

Its a trait, so is the deprecation suggestion being ignored more so because its hard to pull off with a trait method?

[seanmonstar]

I'd be in favor of a deprecation warning, for sure! I think it's trickier for this trait method, since we have to add a default for the new method, and implementors need to include the implementation (I think adding #[allow(deprecated)] on it?)...

[dekellum]

Well, if its reasonably easy to do, I'm in favor of doing it. +2¢