✨ add StrictMerge option and improve list-limit handling in decoder tests#70
Conversation
📝 WalkthroughWalkthroughAdds ChangesList Overflow Handling, StrictMerge, and Bracket-Array Semantics
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #70 +/- ##
==========================================
- Coverage 99.38% 99.30% -0.09%
==========================================
Files 20 20
Lines 2130 2161 +31
Branches 542 552 +10
==========================================
+ Hits 2117 2146 +29
- Misses 13 15 +2
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:
|
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | -12 |
| Duplication | 0 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
QsNet.Tests/DecodeTests.cs (1)
2668-2668: ⚡ Quick winRename new/updated tests to
Should...format for suite consistency.Several new/modified test method names use
Decode_*rather than the requiredShould...naming pattern.As per coding guidelines, “Name tests in
Should...form (e.g.,ShouldDecodeNestedObjects).”Also applies to: 2690-2690, 2718-2718, 2959-2959, 4747-4747, 4772-4772, 4798-4798, 4824-4824, 4866-4866, 4898-4898, 5566-5566, 5588-5588
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@QsNet.Tests/DecodeTests.cs` at line 2668, Rename the test method Decode_Duplicates_BracketNotationAlwaysCombines to follow the suite's "Should..." naming convention (e.g., ShouldCombineDuplicatesWhenUsingBracketNotation) and update any references/usages accordingly; also apply the same rename pattern to the other listed tests (the ones at the indicated ranges) so their names start with "Should" and describe expected behavior (for example, Decode_* -> Should... form) to keep the test suite consistent.QsNet.Tests/EncodeTests.cs (1)
5914-5981: ⚡ Quick winRename new test methods to
Should...form for suite consistency.The newly added test method names use
Encode_...; this breaks the test naming convention used in this project.As per coding guidelines, "Name tests in
Should...form (e.g.,ShouldDecodeNestedObjects); group with nested classes when scenarios grow".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@QsNet.Tests/EncodeTests.cs` around lines 5914 - 5981, Rename the four new test methods to follow the project's "Should..." naming convention while leaving their bodies and [Fact] attributes unchanged: change Encode_StrictNullHandling_FormatsRfc1738BareKey -> Should_FormatRfc1738BareKeyWhenStrictNullHandling, Encode_FilterIterable_SkipsNullEntries -> Should_SkipNullEntriesWhenFilterIterable, Encode_CommaEncodeValuesOnly_HandlesNullEntriesLikeQs -> Should_HandleNullEntriesLikeQsWhenCommaEncodeValuesOnly, and Encode_RoundTripsKeysContainingPercentEncodedBracketText -> Should_RoundTripKeysContainingPercentEncodedBracketText; update only the method names in the test class so references (if any) compile and behavior is identical.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@QsNet.Tests/DecodeTests.cs`:
- Line 2668: Rename the test method
Decode_Duplicates_BracketNotationAlwaysCombines to follow the suite's
"Should..." naming convention (e.g.,
ShouldCombineDuplicatesWhenUsingBracketNotation) and update any
references/usages accordingly; also apply the same rename pattern to the other
listed tests (the ones at the indicated ranges) so their names start with
"Should" and describe expected behavior (for example, Decode_* -> Should...
form) to keep the test suite consistent.
In `@QsNet.Tests/EncodeTests.cs`:
- Around line 5914-5981: Rename the four new test methods to follow the
project's "Should..." naming convention while leaving their bodies and [Fact]
attributes unchanged: change Encode_StrictNullHandling_FormatsRfc1738BareKey ->
Should_FormatRfc1738BareKeyWhenStrictNullHandling,
Encode_FilterIterable_SkipsNullEntries ->
Should_SkipNullEntriesWhenFilterIterable,
Encode_CommaEncodeValuesOnly_HandlesNullEntriesLikeQs ->
Should_HandleNullEntriesLikeQsWhenCommaEncodeValuesOnly, and
Encode_RoundTripsKeysContainingPercentEncodedBracketText ->
Should_RoundTripKeysContainingPercentEncodedBracketText; update only the method
names in the test class so references (if any) compile and behavior is
identical.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2035c00e-c65f-4f61-832e-3087921cdf43
⛔ Files ignored due to path filters (1)
QsNet.Comparison/js/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (11)
QsNet.Comparison/js/package.jsonQsNet.Tests/DecodeOptionsTests.csQsNet.Tests/DecodeTests.csQsNet.Tests/EncodeTests.csQsNet.Tests/UtilsTests.csQsNet/Internal/Decoder.csQsNet/Internal/Utils.csQsNet/Models/DecodeOptions.csQsNet/Qs.csREADME.mddocs/docs/decoding.md
There was a problem hiding this comment.
Pull request overview
This PR updates QsNet’s decode/encode behavior and documentation/tests to better match upstream qs behavior, including new decode semantics around merge conflicts, list limits, and bracket-array duplicate handling, plus a qs JS comparison dependency bump.
Changes:
- Added
DecodeOptions.StrictMerge(defaulttrue) and updated merge behavior/tests/docs for object/primitive conflicts. - Updated decode behavior/tests so
foo[]bracket-array duplicates always combine, and refinedListLimitsemantics (explicit indices and comma-splitting overflow behavior). - Updated encoding output when
CharsetSentinel=trueto use the configured delimiter (not always&), and bumpedqs(JS comparison) to6.15.2.
Reviewed changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| README.md | Documents bracket-array duplicate combining, StrictMerge, and clarified ListLimit semantics. |
| QsNet/Qs.cs | Applies bracket-array duplicate combining to enumerable input bucketing; fixes charset-sentinel/body delimiter joining. |
| QsNet/Models/DecodeOptions.cs | Introduces StrictMerge option and threads it through CopyWith; clarifies ListLimit doc. |
| QsNet/Internal/Utils.cs | Implements StrictMerge merge behavior; adds list-overflow helpers and overflow-aware combining logic. |
| QsNet/Internal/Decoder.cs | Adjusts comma parsing + list-limit enforcement; propagates overflow maps through decoding; explicit-index limit semantics. |
| QsNet.Tests/UtilsTests.cs | Updates merge tests to cover legacy behavior via StrictMerge=false. |
| QsNet.Tests/EncodeTests.cs | Updates delimiter/sentinel expectation and adds additional encode parity/edge-case tests. |
| QsNet.Tests/DecodeTests.cs | Expands decode tests for StrictMerge, bracket-array duplicates, list-limit edges, and comma overflow behavior. |
| QsNet.Tests/DecodeOptionsTests.cs | Extends CopyWith tests to include StrictMerge. |
| QsNet.Comparison/js/pnpm-lock.yaml | Updates locked qs version to 6.15.2. |
| QsNet.Comparison/js/package.json | Bumps qs dependency to ^6.15.2. |
| docs/docs/decoding.md | Mirrors README updates for bracket-array duplicates, StrictMerge, and ListLimit semantics. |
Files not reviewed (1)
- QsNet.Comparison/js/pnpm-lock.yaml: Language not supported
|
Actionable comments posted: 0 |
|
Actionable comments posted: 0 |
techouse
changed the title
StrictMerge option and improve list-limit handling in decoder tests
This pull request primarily updates the
qsnpm package dependency to version 6.15.2 and significantly expands and refines the test suite for theQsNetdecoding logic. The test changes focus on stricter and more accurate handling of array/list limits, merging strategies, and edge cases such as comma-separated values and bracket notation. These improvements help ensure the decoder's behavior closely matches the intended and documented behavior, especially in complex or ambiguous scenarios.Dependency Update:
qsnpm package from version 6.15.1 to 6.15.2 in bothpackage.jsonandpnpm-lock.yaml, ensuring the latest bug fixes and features from the upstream library are included. [1] [2] [3] [4]Decode Options and Merge Behavior:
StrictMergeoption inDecodeOptions, with new tests verifying both the default and legacy (false) behaviors for merging object and primitive values under the same key. [1] [2] [3] [4] [5] [6]Duplicatessetting.Array/List Limit Handling:
ListLimitoption, including correct conversion to maps when limits are exceeded and proper exception throwing whenThrowOnLimitExceededis set. [1] [2] [3] [4] [5]Comma-Separated Value Handling:
Edge Case and Encoding Tests:
These changes enhance the reliability and predictability of the decoder, especially in edge cases and under strict option settings.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation
Chores