🚸 make package netstandard2.0 compatible
#7
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughAdds netstandard2.0 support via multi-targeting and NETSTANDARD2_0 conditional code: polyfill for init-only, regex/encoding/index-range fallbacks, decoder/utils adaptations, delimiter and list-format tweaks, removal of one unit test and an internal ToStringKeyedDictionary helper, and addition of ReferenceEqualityComparer and RegexDelimiter. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Qs as Qs.Decode
participant Decoder
participant Utils
Caller->>Qs: Decode(query, options)
Qs->>Decoder: ParseKeys(key,value,options)
Decoder-->>Qs: parsed (IDictionary or value)
Qs->>Utils: Merge(current, parsed, options)
Utils-->>Qs: merged graph
Qs->>Utils: Compact(merged)
Utils-->>Qs: compacted graph
Qs->>Utils: ToStringKeyDeepNonRecursive(compacted)
Utils-->>Qs: Dictionary<string, object?>
Qs-->>Caller: Result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50–75 minutes Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (2)
QsNet/Models/DecodeOptions.cs (1)
238-238: CopyWith sets Decoder to GetDecoder causing infinite recursionAssigning Decoder = decoder ?? GetDecoder creates a self-referential delegate. GetDecoder checks if Decoder != null and then invokes Decoder, resulting in unbounded recursion.
Apply this diff to preserve existing custom decoders and avoid recursion (mirrors EncodeOptions.Encoder pattern):
- Decoder = decoder ?? GetDecoder, + Decoder = decoder ?? Decoder,Follow-up: Consider adding a unit test that calls new DecodeOptions().CopyWith() without a decoder and then asserts options.GetDecoder("x") delegates to Utils.Decode without recursion.
QsNet/Models/Delimiter.cs (1)
48-66: Fix duplicate ‘Pattern’ property and ensure Regex compiles against the final pattern
- Declaring public string Pattern { get; init; } in the body conflicts with the auto-generated property from the primary constructor and will not compile.
- _rx is initialized from Pattern at field-initializer time. If a consumer uses an object initializer to set Pattern, _rx will be compiled using the constructor argument, not the final value, causing a mismatch.
Apply this diff to remove the duplicate property and lazily initialize the regex from the finalized pattern:
public sealed record RegexDelimiter(string Pattern) : IDelimiter { - private readonly Regex _rx = new(Pattern, RegexOptions.Compiled); + private Regex? _rx; - /// <summary> - /// The regex pattern used for splitting the input string. - /// </summary> - public string Pattern { get; init; } = Pattern; - /// <summary> /// Splits the input string using the regex delimiter. /// </summary> /// <param name="input">The input string to split</param> /// <returns>A list of split strings</returns> public IEnumerable<string> Split(string input) { - return _rx.Split(input); + var rx = _rx ??= new Regex(Pattern, RegexOptions.Compiled); + return rx.Split(input); } }
🧹 Nitpick comments (3)
QsNet/Models/Delimiter.cs (1)
34-38: Prefer explicit array construction over collection expressions for broader compiler compatibilityThe collection expression [Value] requires newer C# compilers. To avoid depending on the latest language features (especially important when multi-targeting), use the explicit array creation.
Apply this diff:
-#if NETSTANDARD2_0 - return Value.Length == 1 ? input.Split(Value[0]) : input.Split([Value], StringSplitOptions.None); -#else +#if NETSTANDARD2_0 + return Value.Length == 1 + ? input.Split(Value[0]) + : input.Split(new[] { Value }, StringSplitOptions.None); +#else return input.Split(Value); #endifQsNet/Internal/Decoder.cs (1)
99-107: Consider extracting the bracket token normalization logic.The bracket token normalization (
%5B→[and%5D→]) appears in both conditional branches with different implementations. Consider extracting this into a helper method to reduce duplication and improve maintainability.+ private static string NormalizeBracketTokens(string input) + { +#if NETSTANDARD2_0 + var result = ReplaceOrdinalIgnoreCase(input, "%5B", "["); + return ReplaceOrdinalIgnoreCase(result, "%5D", "]"); +#else + return input + .Replace("%5B", "[", StringComparison.OrdinalIgnoreCase) + .Replace("%5D", "]", StringComparison.OrdinalIgnoreCase); +#endif + } + internal static Dictionary<string, object?> ParseQueryStringValues( string str, DecodeOptions? options = null ) { options ??= new DecodeOptions(); var obj = new Dictionary<string, object?>(); -#if NETSTANDARD2_0 - var cleanStr = options.IgnoreQueryPrefix ? str.TrimStart('?') : str; - cleanStr = ReplaceOrdinalIgnoreCase(cleanStr, "%5B", "["); - cleanStr = ReplaceOrdinalIgnoreCase(cleanStr, "%5D", "]"); -#else - var cleanStr = (options.IgnoreQueryPrefix ? str.TrimStart('?') : str) - .Replace("%5B", "[", StringComparison.OrdinalIgnoreCase) - .Replace("%5D", "]", StringComparison.OrdinalIgnoreCase); -#endif + var cleanStr = options.IgnoreQueryPrefix ? str.TrimStart('?') : str; + cleanStr = NormalizeBracketTokens(cleanStr);QsNet/Internal/Utils.cs (1)
135-138: Consider using TryAdd for consistency.The code uses both
ContainsKeycheck with indexer assignment andTryAdd. Consider usingTryAddconsistently with appropriate handling.- if (mutable.ContainsKey(j)) - mutable[j] = Merge(mutable[j], item, options); - else - mutable.Add(j, item); + if (!mutable.TryAdd(j, item)) + mutable[j] = Merge(mutable[j], item, options);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (10)
QsNet.Tests/UtilsTests.cs(0 hunks)QsNet/Compat/IsExternalInit.cs(1 hunks)QsNet/Enums/ListFormat.cs(2 hunks)QsNet/Internal/Decoder.cs(10 hunks)QsNet/Internal/Utils.cs(16 hunks)QsNet/Models/DecodeOptions.cs(1 hunks)QsNet/Models/Delimiter.cs(2 hunks)QsNet/Models/EncodeOptions.cs(1 hunks)QsNet/Qs.cs(2 hunks)QsNet/QsNet.csproj(1 hunks)
💤 Files with no reviewable changes (1)
- QsNet.Tests/UtilsTests.cs
🧰 Additional context used
🧬 Code Graph Analysis (4)
QsNet/Models/EncodeOptions.cs (1)
QsNet/Internal/Utils.cs (1)
Equals(1100-1103)
QsNet/Models/DecodeOptions.cs (1)
QsNet/Internal/Utils.cs (1)
Equals(1100-1103)
QsNet/Qs.cs (2)
QsNet/Internal/Decoder.cs (2)
Decoder(16-458)Dictionary(91-218)QsNet/Internal/Utils.cs (9)
Utils(20-1089)Merge(69-249)Dictionary(505-646)Dictionary(803-809)Dictionary(816-825)Dictionary(832-844)Dictionary(893-896)Dictionary(898-964)Dictionary(992-1088)
QsNet/Internal/Decoder.cs (3)
QsNet/Internal/Utils.cs (8)
Regex(36-39)Regex(51-54)GeneratedRegex(41-42)GeneratedRegex(56-57)Utils(20-1089)Apply(690-698)IsEmpty(724-734)InterpretNumericEntities(741-796)QsNet/Enums/Sentinel.cs (2)
GetEncoded(50-58)ToString(65-68)QsNet/Models/DecodeOptions.cs (1)
GetDecoder(178-181)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Coverage (merged)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Codacy Security Scan
- GitHub Check: Analyze (csharp)
🔇 Additional comments (12)
QsNet/Compat/IsExternalInit.cs (1)
1-11: Good polyfill for init-only setters on netstandard2.0The NETSTANDARD2_0-gated IsExternalInit is minimal, correctly scoped, and avoids impacting other targets. LGTM.
QsNet/Enums/ListFormat.cs (1)
44-48: Precomputed generators remove per-call allocations and clarify intentUsing static readonly delegates avoids re-allocating lambdas on each call to GetGenerator. Mapping via the switch is clean and preserves semantics. LGTM.
Also applies to: 58-61
QsNet/QsNet.csproj (1)
3-3: LGTM! Multi-target configuration correctly enables .NET Standard 2.0 support.The change from
<TargetFramework>to<TargetFrameworks>withnet8.0;netstandard2.0appropriately supports backward compatibility while maintaining modern .NET 8.0 support.QsNet/Qs.cs (1)
64-89: LGTM! Proper compatibility handling for .NET Standard 2.0.The conditional compilation correctly handles the tuple deconstruction limitation in .NET Standard 2.0 by using explicit
kv.Keyandkv.Valueaccess instead of the tuple deconstruction pattern(key, value). The logic remains identical between both code paths.QsNet/Internal/Decoder.cs (4)
15-19: LGTM! Correct class declaration for .NET Standard 2.0 compatibility.The conditional compilation properly handles the class declaration, making it
staticfor .NET Standard 2.0 (which lacks source generators) andpartialfor newer targets to supportGeneratedRegex.
27-36: LGTM! Well-implemented regex caching for .NET Standard 2.0.The implementation correctly provides a cached regex instance for .NET Standard 2.0 while using the more efficient
GeneratedRegexfor newer targets. TheRegexOptions.Compiledflag is appropriately used for performance.
38-50: LGTM! Clean abstraction for Latin-1 encoding across targets.The implementation provides a clean abstraction for Latin-1 encoding differences between targets. The
IsLatin1helper method correctly identifies Latin-1 encoding using code page 28591 for .NET Standard 2.0 and directEncoding.Latin1comparison for newer targets.
432-456: LGTM! Efficient case-insensitive string replacement for .NET Standard 2.0.The custom
ReplaceOrdinalIgnoreCaseimplementation is well-optimized with minimal allocations. It correctly usesStringBuilderlazily (only when replacements are found) and handles all edge cases properly.QsNet/Internal/Utils.cs (4)
19-23: LGTM! Proper conditional class declaration.The conditional compilation correctly handles the class declaration for different targets.
119-124: LGTM! Excellent optimization to avoid multiple enumeration.Materializing the source enumerable to a list is a good defensive practice that prevents multiple enumeration of potentially expensive sequences while maintaining the same behavior.
365-366: LGTM! Good defensive null handling.Using
format.GetValueOrDefault()and the non-null assertion withstr!after the null/empty check improves code clarity and null safety.
1091-1109: LGTM! Well-implemented reference equality comparer.The
ReferenceEqualityCompareris correctly implemented as a sealed singleton with proper use ofRuntimeHelpers.GetHashCodefor consistent hash codes based on object identity rather than value equality. This is essential for tracking visited objects in circular reference scenarios.
1 participant
This pull request introduces comprehensive compatibility improvements for
netstandard2.0, refactors utility and decoder logic for better maintainability, and optimizes handling of collections and string operations. The most important changes are grouped below..NET Standard 2.0 Compatibility
IsExternalInitinQsNet/Compat/IsExternalInit.csto support init-only setters onnetstandard2.0.DecoderandUtilsclasses to use conditional compilation (#if NETSTANDARD2_0) for regex usage, string slicing, and encoding operations to ensure compatibility withnetstandard2.0. [1] [2] [3]Decoder Logic Enhancements
Decoder.csto use compatible string slicing and encoding checks, including a custom case-insensitive string replace for query parameter normalization. [1] [2] [3] [4] [5] [6]Utility Method Refactoring
Utils.csby materializing enumerables to lists before processing, preventing multiple enumerations and improving performance. [1] [2] [3]netstandard2.0and newer frameworks, including handling of Unicode and hex escapes. [1] [2] [3] [4] [5]ListFormat Extension Improvements
ListFormatExtensionsto use static generator delegates for each format, reducing allocations and improving clarity. [1] [2]Test Suite Cleanup
ToStringKeyedDictionary_Converts_Keys_To_StringfromUtilsTests.csto streamline the test suite.Summary by CodeRabbit