🩹 Fix: sorting error in sortAcceptedTypes #3331
Conversation
WalkthroughThe changes update the handling of the Changes
Sequence Diagram(s)sequenceDiagram
participant Offer as getOffer
participant Sort as sortAcceptedTypes
Offer->>Sort: Call sortAcceptedTypes(acceptedTypes slice)
Note right of Sort: Evaluate slice (length/checks) and perform sorting
Sort-->>Offer: Return sorted slice
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (9)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3331 +/- ##
==========================================
+ Coverage 84.21% 84.27% +0.06%
==========================================
Files 116 116
Lines 11577 11572 -5
==========================================
+ Hits 9749 9752 +3
+ Misses 1397 1391 -6
+ Partials 431 429 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
ksw2000
requested review from
gaby,
sixcolors,
ReneWerner87 and
efectn
and removed request for
a team
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
helpers.go(2 hunks)helpers_test.go(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: unit (1.24.x, macos-13)
- GitHub Check: unit (1.24.x, macos-latest)
- GitHub Check: unit (1.24.x, windows-latest)
- GitHub Check: unit (1.23.x, macos-13)
- GitHub Check: unit (1.23.x, windows-latest)
- GitHub Check: repeated
- GitHub Check: Analyse
- GitHub Check: Compare
🔇 Additional comments (5)
helpers_test.go (3)
365-367: Approved change to fix the sorting order test case.The change adds an application/json entry with order 3 at the end of the slice and modifies the function call to directly pass the slice instead of a pointer, aligning with the function signature change in the implementation file.
393-393: Properly updated benchmark call.The benchmark function has been updated to correctly pass the slice directly instead of a pointer, consistent with the function signature change.
417-417: Properly updated unsorted benchmark call.The unsorted benchmark function has also been updated to correctly pass the slice directly instead of a pointer, maintaining consistency with the function signature change.
helpers.go (2)
486-486: Updated function call to match new signature.The call to
sortAcceptedTypesnow correctly passes the slice directly rather than its address, aligning with the updated function signature.
514-517: Improved function signature for better Go idioms.The function now accepts a slice directly instead of a pointer to a slice, which is more idiomatic in Go. This improves readability and reduces unnecessary pointer operations.
|
|
||
| for i := 1; i < len(at); i++ { | ||
| lo, hi := 0, i-1 | ||
| for lo <= hi { | ||
| mid := (lo + hi) / 2 | ||
| if at[i].quality < at[mid].quality || | ||
| (at[i].quality == at[mid].quality && at[i].specificity < at[mid].specificity) || | ||
| (at[i].quality == at[mid].quality && at[i].specificity < at[mid].specificity && len(at[i].params) < len(at[mid].params)) || | ||
| (at[i].quality == at[mid].quality && at[i].specificity == at[mid].specificity && len(at[i].params) < len(at[mid].params)) || |
There was a problem hiding this comment.
Fixed sorting condition for params comparison.
The comparison logic has been fixed to properly sort items when they have the same quality and specificity by checking if one has fewer parameters than the other. This precise change resolves the sorting error mentioned in the PR objectives.
Before this fix, the comparison logic likely didn't correctly handle the sorting of entries with different parameter counts, causing the element with order: 3 to be placed incorrectly.
can you share benchmark results for this |
|
|
He shared the article https://stackoverflow.com/questions/72978660/is-it-correct-to-use-slice-as-item-because-slice-is-by-default-pointer with me |
|
CI is randomly failing but it's not related to this PR. Triggered "replay failed jobs" |
Description
The original implementation fails in the test below. The test is modified simply by moving
{spec: "application/json", quality: 0.999, specificity: 3, order: 3}to the last element.In addition, the argument of
sortAcceptedTypescan be aslicerather thana pointer to a slice.Type of change
Checklist
Before you submit your pull request, please make sure you meet these requirements:
/docs/directory for Fiber's documentation.