chore: dedupe SSE optional-space strip and simplify retry/grow helpers#190
Merged
Merged
Conversation
Three behavior-preserving cleanups in ServerSentEventReader, all internal:
- Fold the WHATWG SSE §9.2.6 "drop one optional leading space" rule, which
the comment path and the field-value path each implemented separately, into
a single private valueFrom(line, start) helper. valueFrom(line, 1) reproduces
the old substring(1).removePrefix(" ") for every input and drops its extra
intermediate allocation.
- Replace the hand-rolled base-10 parse + manual overflow check in
parseRetryMillis with toLongOrNull behind an explicit ASCII-digit guard. The
guard (not a filter) preserves spec semantics: signs and embedded non-digits
are still rejected, empty input still yields null, and toLongOrNull rejects
values past Long.MAX_VALUE at the identical boundary. Removes the now-unused
DECIMAL_BASE constant.
- Drop the unused minCapacity parameter and unreachable else arm from
ByteArrayBuilder.grow. Its sole caller always passes bytes.size + 1, so the
growth reduces to "first allocation is INITIAL_CAP, otherwise double" with the
same 0 -> 64 -> 128 -> ... sequence.
Output stays byte-identical; the existing exact-equality test suite is unchanged
apart from refreshing one test comment that named a now-removed internal branch.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Three small, behavior-preserving cleanups in
ServerSentEventReader, all confined to private internals. Parsing semantics are unchanged: the WHATWG SSE rules and the byte-identical output pinned by the exact-equality test suite stay exactly as they are.1. Share one optional-leading-space strip between comments and field values
The comment path (
substring(1).removePrefix(" ")) and the field-value path (an inlineifon the byte after the colon) each implemented the same "drop one optional U+0020 after the colon" rule from WHATWG SSE §9.2.6. They now go through a single privatevalueFrom(line, start)helper.valueFrom(line, 1)reproduces the old comment behavior for every input (only-colon, one space, two spaces, none) and drops the extra intermediate allocation thatremovePrefix(" ")added on top ofsubstring(1).2. Parse
retry:with the stdlib behind a digit guardparseRetryMillishand-rolled a base-10 parse with manual overflow detection — exactly whatString.toLongOrNull()already does. It now guards with an explicit ASCII-digit check and delegates the parse. The guard stays a guard, not a filter: a filter would parse"1a2"as12, andtoLongOrNullalone would accept a leading+/-the spec forbids. Empty input still returnsnull, and values pastLong.MAX_VALUEare still rejected at the identical boundary. The now-unusedDECIMAL_BASEconstant is removed.3. Leaner
ByteArrayBuilder.growgrowis only ever called fromappend, always withminCapacity == bytes.size + 1. Under that single contract theelse -> oldCaparm is unreachable and the parameter carries no information. Reducing it to "first allocation isINITIAL_CAP, otherwise double" keeps the same0 → 64 → 128 → …growth sequence with less code.Evaluation of the original suggestions
I reviewed each proposed change against the source and the test suite before implementing. All three are correct and behavior-preserving — the digit-guard reasoning in particular (guard vs. filter, identical overflow boundary) checks out against the existing
retryedge-case tests (negative sign, embedded non-digit, empty value, exactlyLong.MAX_VALUE, one above, 30-digit overflow). One thing the suggestions missed: two test comments described the now-removed internal overflow branch (result > (Long.MAX_VALUE - digit) / 10), so one was refreshed to describe the behavior rather than the deleted implementation detail.Testing
./gradlew :sdk-core:detekt :sdk-core:ktlintMainSourceSetCheck :sdk-core:ktlintTestSourceSetCheck :sdk-core:test --tests "org.dexpace.sdk.core.http.sse.*"— all pass.Closes #176