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.

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

Make BytesMut::try_unsplit method public #746

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

petrosagg
Copy link

This PR is identical diff-wise to #513 with some additional justification that hopefully leads to a merge.

There are currently two open discussions. One is on whether the Bytes type should have a try_unsplit method and another one is on whether the BytesMut type should have a try_unsplit method. The former is being discussed in #287 and the latter in #702.

The discussion on Bytes is complicated by the fact that the type does not currently have unsplit functionality. Therefore introducing it for that type is something to be considered deeply since it expands the surface area and comes with maintainability and backwards compatibility concerns.

This PR is NOT related to the Bytes type or that discussion.

This PR only changes BytesMut which already offers an unsplit method in its public API whose specification (i.e doc comment) is:

Absorbs a BytesMut that was previously split off if they are contiguous, otherwise appends its bytes to this BytesMut.

This means that at least for BytesMut the unsplit functionality is here to stay, at least until a new major version is released. Also, the specification of unsplit already encodes this idea of "it will try to do X, otherwise fallback to Y" and so it is reasonable to expect that any future changes in the codebase will have to maintain this kind of conditional implementation where at some point the unsplit function makes a decision.

Under that view, all this PR is doing is offering what unsplit already does but without the "otherwise..." part, leaving it up to the user to select what happens next. This shouldn't present any backward compatibility/new surface concerns since no matter how unsplit evolves we can always take the logic until the if possible_to_merge { .. } bit and expose it under try_unsplit

@petrosagg
Copy link
Author

Linking also some previous agreement on offering both flavors of this API #445

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants