feature: add AddTrailingSlash #162
Conversation
duhnnie
requested review from
ReneWerner87,
efectn,
gaby and
sixcolors
and removed request for
a team
Summary of ChangesHello @duhnnie, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a new generic utility function, Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new utility function, AddTrailingSlash, along with its unit tests and benchmarks. The implementation is solid and the tests provide good coverage. My review includes a few suggestions to improve code clarity and maintainability, such as correcting typos in comments and test logs, removing a redundant type assertion, and a recommendation to document a potential side-effect in the []byte handling to prevent misuse.
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThis change introduces two new utility functions— Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
byteseq.go (1)
93-95: Remove unnecessary type assertion.When
valready ends with '/', the type assertionany(v).(T)is redundant sincevis already of typeT.Apply this diff:
if len(v) > 0 && v[len(v)-1] == '/' { - return any(v).(T) + return v }byteseq_test.go (1)
349-385: Add allocation reporting to benchmark for consistency.All other benchmarks in this file call
b.ReportAllocs()to track memory allocations, which is important for performance analysis. This benchmark should follow the same pattern.Apply this diff:
for _, tt := range tests { b.Run(tt.name, func(b *testing.B) { + b.ReportAllocs() switch v := tt.in.(type) { case string:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
byteseq.go(1 hunks)byteseq_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
byteseq_test.go (1)
byteseq.go (1)
AddTrailingSlash(92-106)
duhnnie
force-pushed
the
feature/addtrailingslash
branch
from
3865bdf to
df3a88e
Compare
|
@ReneWerner87 isn't the trailing slash matching handled by fasthttp? |
|
@gaby Yes, that's right, but this helper is used to compare the paths that we have stored in the routing tree of the various apps (mount paths) and thus prioritise which error handler is used by the mounted app. |
duhnnie
force-pushed
the
feature/addtrailingslash
branch
from
df3a88e to
5f61c4b
Compare
|
@ReneWerner87 @gaby I made corrections to the initial solutions (docs, linting, typos, improvements, etc) |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
byteseq.go(1 hunks)byteseq_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- byteseq_test.go
🔇 Additional comments (1)
byteseq.go (1)
104-116: LGTM: Type switch implementation correctly addresses mutation concerns.The implementation properly handles both string and
[]bytecases:
- String concatenation is idiomatic and creates a new string
[]byteallocation with explicitmakeandcopyavoids the mutation issues raised in past reviews- The
nolintdirectives are appropriate since the type assertions are guaranteed by the constraint
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #162 +/- ##
==========================================
+ Coverage 84.62% 84.81% +0.19%
==========================================
Files 12 12
Lines 956 968 +12
==========================================
+ Hits 809 821 +12
Misses 126 126
Partials 21 21
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Waiting for your decision about this: #162 (comment) |
|
@duhnnie The benchmarks are showing that when the string doesn't have a trailing slash, it's very inefficient adding one at the end (your switch case) |
duhnnie
force-pushed
the
feature/addtrailingslash
branch
from
5f61c4b to
46680cd
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
byteseq_test.go (1)
343-378: Consider adding baseline comparisons for performance context.The benchmark comprehensively tests various input sizes and conditions. However, it might be valuable to add comparison benchmarks against:
- Naive string concatenation (e.g.,
if !strings.HasSuffix(s, "/") { s += "/" })- Direct type-specific implementations without generics
This would help quantify the performance trade-off of the generic approach and inform the decision about whether to keep the current implementation or switch to specialized functions.
Example addition:
func Benchmark_AddTrailingSlash_Comparison(b *testing.B) { input := "abc" b.Run("Generic", func(b *testing.B) { for i := 0; i < b.N; i++ { _ = AddTrailingSlash(input) } }) b.Run("DirectString", func(b *testing.B) { for i := 0; i < b.N; i++ { s := input if len(s) > 0 && s[len(s)-1] != '/' { s = s + "/" } _ = s } }) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
byteseq.go(1 hunks)byteseq_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
byteseq_test.go (1)
byteseq.go (1)
AddTrailingSlash(101-118)
🔇 Additional comments (4)
byteseq.go (2)
91-100: Documentation is clear and accurate.The documentation accurately describes the behavior for both string and []byte inputs, including the optimization that returns the original input when no modification is needed.
106-118: Reconsider the type switch approach if benchmarks show significant performance degradation.The generic type conversions using
any()do introduce measurable overhead—this is well-documented in Go: type assertions on interface values are slower than concrete types or properly constrained generics, with observed slowdowns of tens to hundreds of percent in tight loops.If benchmarks confirm material performance impact for this code path, consider whether:
- Separate type-specific functions (
AddTrailingSlashString,AddTrailingSlashBytes) are worth the API trade-off- The constraint design allows a more efficient generic approach
- This code path is performance-critical for typical use cases
However, decide based on actual benchmark data from this codebase rather than theoretical concerns alone.
byteseq_test.go (2)
307-323: Test coverage is comprehensive and correct.The test cases cover all important scenarios: empty input, input without trailing slash, input with trailing slash, and the root case. The use of
require.Equalis consistent with the rest of the file.
325-341: Byte slice test is correct and mirrors string test coverage.This test appropriately validates the []byte variant with the same comprehensive test cases as the string version.
duhnnie
force-pushed
the
feature/addtrailingslash
branch
from
46680cd to
bb03b21
Compare
|
I see, you're right, the main issue is the allocation cost for large strings. However, since strings in Go are immutable, adding a trailing slash always requires allocating a new string. This is expected and unavoidable. For the []byte case, we could check the slice capacity and append the slash without allocating, but that would mutate the caller’s buffer. Because this function is intended to be non-mutating and behave consistently for both string and []byte inputs, we intentionally allocate a new slice instead. About the use of the switch, I left this comment above: #162 (comment) Thoughts? |
|
@duhnnie See if it works using utils.UnsafeString and utils.UnsafeBytes, since in fiber we use those |
|
/gemini review |
|
@gaby can you check again |
There was a problem hiding this comment.
Code Review
This pull request introduces a new utility function, AddTrailingSlash, and its associated tests. The core logic in AddTrailingSlash has a critical flaw in its handling of generic types, which I've detailed in my comments. Specifically, it doesn't correctly process named types that have string or []byte as their underlying type. I've also identified a couple of areas for improvement in the tests to make them more robust, including testing for named types and ensuring slice identity is checked correctly.
|
bench output from the bench action Benchmark_AddTrailingSlash/string/empty-4 68699278 17.43 ns/op 1 B/op 1 allocs/op
Benchmark_AddTrailingSlash/string/slash-only-4 501702490 2.391 ns/op 418.15 MB/s 0 B/op 0 allocs/op
Benchmark_AddTrailingSlash/string/short-no-slash-4 58627597 18.93 ns/op 158.48 MB/s 4 B/op 1 allocs/op
Benchmark_AddTrailingSlash/string/short-with-slash-4 501861198 2.391 ns/op 1672.99 MB/s 0 B/op 0 allocs/op
Benchmark_AddTrailingSlash/string/path-no-slash-4 41579101 27.43 ns/op 474.01 MB/s 16 B/op 1 allocs/op
Benchmark_AddTrailingSlash/string/path-with-slash-4 502002145 2.391 ns/op 5854.90 MB/s 0 B/op 0 allocs/op
Benchmark_AddTrailingSlash/string/long-no-slash-4 34721426 32.61 ns/op 1349.08 MB/s 48 B/op 1 allocs/op
Benchmark_AddTrailingSlash/string/long-with-slash-4 501992530 2.390 ns/op 18826.44 MB/s 0 B/op 0 allocs/op
Benchmark_AddTrailingSlashBytes/bytes/empty-4 47151802 23.87 ns/op 8 B/op 1 allocs/op
Benchmark_AddTrailingSlashBytes/bytes/slash-only-4 501587596 2.394 ns/op 417.78 MB/s 0 B/op 0 allocs/op
Benchmark_AddTrailingSlashBytes/bytes/short-no-slash-4 350623000 3.426 ns/op 875.54 MB/s 0 B/op 0 allocs/op
Benchmark_AddTrailingSlashBytes/bytes/short-with-slash-4 501496095 2.396 ns/op 1669.56 MB/s 0 B/op 0 allocs/op
Benchmark_AddTrailingSlashBytes/bytes/path-no-slash-4 350578702 3.428 ns/op 3792.37 MB/s 0 B/op 0 allocs/op
Benchmark_AddTrailingSlashBytes/bytes/path-with-slash-4 501639562 2.398 ns/op 5837.43 MB/s 0 B/op 0 allocs/op
Benchmark_AddTrailingSlashBytes/bytes/long-no-slash-4 350547199 3.424 ns/op 12848.77 MB/s 0 B/op 0 allocs/op
Benchmark_AddTrailingSlashBytes/bytes/long-with-slash-4 499894323 2.394 ns/op 18794.15 MB/s 0 B/op 0 allocs/op |
This is bad, why is appending a slash taking 18ns? |
…with tests for trailing slash handling
Benchmark_AddTrailingSlashBytes
Benchmark_AddTrailingSlashBytes/empty
Benchmark_AddTrailingSlashBytes/empty-12 1000000000 0.5934 ns/op 0 B/op 0 allocs/op
Benchmark_AddTrailingSlashBytes/empty-12 1000000000 0.6035 ns/op 0 B/op 0 allocs/op
Benchmark_AddTrailingSlashBytes/empty-12 1000000000 0.6054 ns/op 0 B/op 0 allocs/op
Benchmark_AddTrailingSlashBytes/empty-12 1000000000 0.6011 ns/op 0 B/op 0 allocs/op
Benchmark_AddTrailingSlashBytes/empty-12 1000000000 0.5932 ns/op 0 B/op 0 allocs/op
Benchmark_AddTrailingSlashBytes/slash-only
Benchmark_AddTrailingSlashBytes/slash-only-12 1000000000 0.8688 ns/op 0 B/op 0 allocs/op
Benchmark_AddTrailingSlashBytes/slash-only-12 1000000000 0.8784 ns/op 0 B/op 0 allocs/op
Benchmark_AddTrailingSlashBytes/slash-only-12 1000000000 0.8779 ns/op 0 B/op 0 allocs/op
Benchmark_AddTrailingSlashBytes/slash-only-12 1000000000 0.8898 ns/op 0 B/op 0 allocs/op
Benchmark_AddTrailingSlashBytes/slash-only-12 1000000000 0.8919 ns/op 0 B/op 0 allocs/op
Benchmark_AddTrailingSlashBytes/short-no-slash
Benchmark_AddTrailingSlashBytes/short-no-slash-12 100000000 11.22 ns/op 4 B/op 1 allocs/op
Benchmark_AddTrailingSlashBytes/short-no-slash-12 100000000 11.39 ns/op 4 B/op 1 allocs/op
Benchmark_AddTrailingSlashBytes/short-no-slash-12 100000000 11.02 ns/op 4 B/op 1 allocs/op
Benchmark_AddTrailingSlashBytes/short-no-slash-12 100000000 11.32 ns/op 4 B/op 1 allocs/op
Benchmark_AddTrailingSlashBytes/short-no-slash-12 100000000 11.10 ns/op 4 B/op 1 allocs/op
Benchmark_AddTrailingSlashBytes/short-with-slash
Benchmark_AddTrailingSlashBytes/short-with-slash-12 1000000000 0.8768 ns/op 0 B/op 0 allocs/op
Benchmark_AddTrailingSlashBytes/short-with-slash-12 1000000000 0.8781 ns/op 0 B/op 0 allocs/op
Benchmark_AddTrailingSlashBytes/short-with-slash-12 1000000000 0.8670 ns/op 0 B/op 0 allocs/op
Benchmark_AddTrailingSlashBytes/short-with-slash-12 1000000000 0.8755 ns/op 0 B/op 0 allocs/op
Benchmark_AddTrailingSlashBytes/short-with-slash-12 1000000000 0.8831 ns/op 0 B/op 0 allocs/op
Benchmark_AddTrailingSlashBytes/path-no-slash
Benchmark_AddTrailingSlashBytes/path-no-slash-12 91264586 12.74 ns/op 16 B/op 1 allocs/op
Benchmark_AddTrailingSlashBytes/path-no-slash-12 96365540 12.58 ns/op 16 B/op 1 allocs/op
Benchmark_AddTrailingSlashBytes/path-no-slash-12 95957455 12.71 ns/op 16 B/op 1 allocs/op
Benchmark_AddTrailingSlashBytes/path-no-slash-12 94733720 12.87 ns/op 16 B/op 1 allocs/op
Benchmark_AddTrailingSlashBytes/path-no-slash-12 93020551 12.79 ns/op 16 B/op 1 allocs/op
Benchmark_AddTrailingSlashBytes/path-with-slash
Benchmark_AddTrailingSlashBytes/path-with-slash-12 1000000000 0.8892 ns/op 0 B/op 0 allocs/op
Benchmark_AddTrailingSlashBytes/path-with-slash-12 1000000000 0.8913 ns/op 0 B/op 0 allocs/op
Benchmark_AddTrailingSlashBytes/path-with-slash-12 1000000000 0.8770 ns/op 0 B/op 0 allocs/op
Benchmark_AddTrailingSlashBytes/path-with-slash-12 1000000000 0.8698 ns/op 0 B/op 0 allocs/op
Benchmark_AddTrailingSlashBytes/path-with-slash-12 1000000000 0.8697 ns/op 0 B/op 0 allocs/op
Benchmark_AddTrailingSlashString
Benchmark_AddTrailingSlashString/empty
Benchmark_AddTrailingSlashString/empty-12 1000000000 0.2882 ns/op 0 B/op 0 allocs/op
Benchmark_AddTrailingSlashString/empty-12 1000000000 0.2891 ns/op 0 B/op 0 allocs/op
Benchmark_AddTrailingSlashString/empty-12 1000000000 0.2883 ns/op 0 B/op 0 allocs/op
Benchmark_AddTrailingSlashString/empty-12 1000000000 0.2873 ns/op 0 B/op 0 allocs/op
Benchmark_AddTrailingSlashString/empty-12 1000000000 0.2897 ns/op 0 B/op 0 allocs/op
Benchmark_AddTrailingSlashString/slash-only
Benchmark_AddTrailingSlashString/slash-only-12 1000000000 0.4371 ns/op 0 B/op 0 allocs/op
Benchmark_AddTrailingSlashString/slash-only-12 1000000000 0.4360 ns/op 0 B/op 0 allocs/op
Benchmark_AddTrailingSlashString/slash-only-12 1000000000 0.4387 ns/op 0 B/op 0 allocs/op
Benchmark_AddTrailingSlashString/slash-only-12 1000000000 0.4472 ns/op 0 B/op 0 allocs/op
Benchmark_AddTrailingSlashString/slash-only-12 1000000000 0.4364 ns/op 0 B/op 0 allocs/op
Benchmark_AddTrailingSlashString/short-no-slash
Benchmark_AddTrailingSlashString/short-no-slash-12 100000000 10.75 ns/op 4 B/op 1 allocs/op
Benchmark_AddTrailingSlashString/short-no-slash-12 100000000 10.66 ns/op 4 B/op 1 allocs/op
Benchmark_AddTrailingSlashString/short-no-slash-12 100000000 10.73 ns/op 4 B/op 1 allocs/op
Benchmark_AddTrailingSlashString/short-no-slash-12 100000000 10.67 ns/op 4 B/op 1 allocs/op
Benchmark_AddTrailingSlashString/short-no-slash-12 100000000 10.89 ns/op 4 B/op 1 allocs/op
Benchmark_AddTrailingSlashString/short-with-slash
Benchmark_AddTrailingSlashString/short-with-slash-12 1000000000 0.4475 ns/op 0 B/op 0 allocs/op
Benchmark_AddTrailingSlashString/short-with-slash-12 1000000000 0.4394 ns/op 0 B/op 0 allocs/op
Benchmark_AddTrailingSlashString/short-with-slash-12 1000000000 0.4340 ns/op 0 B/op 0 allocs/op
Benchmark_AddTrailingSlashString/short-with-slash-12 1000000000 0.4347 ns/op 0 B/op 0 allocs/op
Benchmark_AddTrailingSlashString/short-with-slash-12 1000000000 0.4342 ns/op 0 B/op 0 allocs/op
Benchmark_AddTrailingSlashString/path-no-slash
Benchmark_AddTrailingSlashString/path-no-slash-12 97934535 12.39 ns/op 16 B/op 1 allocs/op
Benchmark_AddTrailingSlashString/path-no-slash-12 96036816 12.41 ns/op 16 B/op 1 allocs/op
Benchmark_AddTrailingSlashString/path-no-slash-12 99526560 12.30 ns/op 16 B/op 1 allocs/op
Benchmark_AddTrailingSlashString/path-no-slash-12 97432910 12.28 ns/op 16 B/op 1 allocs/op
Benchmark_AddTrailingSlashString/path-no-slash-12 96607342 12.28 ns/op 16 B/op 1 allocs/op
Benchmark_AddTrailingSlashString/path-with-slash
Benchmark_AddTrailingSlashString/path-with-slash-12 1000000000 0.4345 ns/op 0 B/op 0 allocs/op
Benchmark_AddTrailingSlashString/path-with-slash-12 1000000000 0.4342 ns/op 0 B/op 0 allocs/op
Benchmark_AddTrailingSlashString/path-with-slash-12 1000000000 0.4334 ns/op 0 B/op 0 allocs/op
Benchmark_AddTrailingSlashString/path-with-slash-12 1000000000 0.4336 ns/op 0 B/op 0 allocs/op
Benchmark_AddTrailingSlashString/path-with-slash-12 1000000000 0.4328 ns/op 0 B/op 0 allocs/op |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
strings_test.go (2)
197-222: Parallel subtests capture range variabletc; consider rebinding for pre‑Go 1.22 compatibility.With
t.Parallel()inside the subtests, capturing the range variable directly (for _, tc := range testCases) relies on Go 1.22’s new per‑iteration loop semantics. If this module is ever tested with Go < 1.22 (or withloopvardisabled), all subtests may see the sametcvalue.To make the tests robust across Go versions, you can rebind
tcinside the loop:- for _, tc := range testCases { - t.Run(tc.name, func(t *testing.T) { - t.Parallel() - result := AddTrailingSlashString(tc.in) - require.Equal(t, tc.want, result) - }) - } + for _, tc := range testCases { + tc := tc // avoid capturing the loop variable in parallel subtests on Go < 1.22 + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + result := AddTrailingSlashString(tc.in) + require.Equal(t, tc.want, result) + }) + }If you’ve standardized on Go 1.22+ with the new semantics, this becomes optional but still makes intent explicit.
224-247: Optional: set per‑case byte size inBenchmark_AddTrailingSlashString.The benchmark looks good and will surface allocs clearly. For consistency with the existing case-conversion benchmarks (which call
b.SetBytes), you could also set the processed byte count per iteration:- for _, tc := range cases { - b.Run(tc.name, func(b *testing.B) { - b.ReportAllocs() + for _, tc := range cases { + b.Run(tc.name, func(b *testing.B) { + b.ReportAllocs() + b.SetBytes(int64(len(tc.input)))Not required, but it makes throughput numbers more interpretable in
go test -benchmemoutput.bytes_test.go (2)
109-133: As with strings, parallel subtests capture range variabletc; consider a local copy.
Test_AddTrailingSlashBytesusest.Parallel()inside subtests and capturestcfrom thefor _, tc := range testCasesloop. This is only automatically safe under Go 1.22+ loop semantics; on earlier Go versions it can lead to all subtests observing the finaltcvalue.To make the test robust across Go versions, you can rebind
tcinside the loop:- for _, tc := range testCases { - t.Run(tc.name, func(t *testing.T) { - t.Parallel() - result := AddTrailingSlashBytes(tc.in) - require.Equal(t, tc.want, result) - }) - } + for _, tc := range testCases { + tc := tc // avoid capturing loop variable in parallel subtests on Go < 1.22 + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + result := AddTrailingSlashBytes(tc.in) + require.Equal(t, tc.want, result) + }) + }If the module is guaranteed to be built and tested only with Go 1.22+ (and
go 1.22+ in go.mod), this is an optional safety/clarity improvement.
157-180: Optional: callSetBytesinBenchmark_AddTrailingSlashBytesfor clearer throughput metrics.Benchmark correctly reports allocations and exercises multiple input shapes. For consistency with other benchmarks in this package, you could also set the bytes processed per iteration:
- for _, tc := range cases { - b.Run(tc.name, func(b *testing.B) { - b.ReportAllocs() + for _, tc := range cases { + b.Run(tc.name, func(b *testing.B) { + b.ReportAllocs() + b.SetBytes(int64(len(tc.input)))This is purely for nicer
-benchmemoutput; behavior-wise the benchmark is already fine.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
Makefile(1 hunks)bytes.go(1 hunks)bytes_test.go(1 hunks)strings.go(1 hunks)strings_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
strings_test.go (1)
strings.go (1)
AddTrailingSlashString(55-67)
bytes.go (2)
convert.go (2)
UnsafeBytes(26-29)UnsafeString(18-23)strings.go (1)
AddTrailingSlashString(55-67)
bytes_test.go (1)
bytes.go (1)
AddTrailingSlashBytes(70-72)
🔇 Additional comments (3)
strings.go (1)
52-67: AddTrailingSlashString implementation and allocation behavior look solid.Logic is straightforward and efficient:
""→"/"(special-cased).- If last byte is
'/', returns the original string (no alloc).- Otherwise allocates
n+1bytes, copies, appends'/', and returns viaUnsafeStringon a fresh buffer.This matches the documented behavior and the new tests, while ensuring no mutation of the input and allocations only when a slash is actually appended.
bytes.go (1)
67-72: AddTrailingSlashBytes correctly reuses string helper and keeps input immutable.
AddTrailingSlashBytescleanly delegates all logic toAddTrailingSlashStringviaUnsafeString/UnsafeBytes:
- If a slash is already present, it returns a slice backed by the original array with zero allocations.
- If a slash must be appended, the only allocation is from
AddTrailingSlashString’s internal buffer; the original slice is untouched.This matches the doc comment and the new tests around non-mutation and “same backing slice when already slashed”.
bytes_test.go (1)
136-155: Non-mutation and same-backing-slice tests clearly encode the intended API contract.
Test_AddTrailingSlashBytes_NoMutationandTest_AddTrailingSlashBytes_ReturnsSamenicely pin down two important behaviors:
- The original input slice must not be mutated when a slash is appended.
- When a slash is already present, the returned slice shares the same backing array (
require.Same(t, &input[0], &result[0])), which aligns with the currentAddTrailingSlashBytesimplementation using unsafe conversions.This is a good level of specification for the helper’s behavior.
3 participants
This adds a necessary util function (which adds a trailing slash
/to a string if it doesn't have one) for fixing an issue in fiber V3.Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.