🐛 preserve percent-encoded dots in keys during decoding#23
Conversation
…nd for key/value context
WalkthroughAdds DecodeKind enum and re-exports it; makes decode/loads accept optional per-call options and clone them to avoid mutating caller state; adds a decoder adapter in DecodeOptions to support legacy and kind-aware decoders; makes DecodeUtils.decode kind-aware (KEY vs VALUE); expands tests for these behaviors. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant decode as decode()
participant opts as DecodeOptions (adapter)
participant decoder as decoder_fn (adapter)
participant utils as DecodeUtils.decode
Caller->>decode: decode(value, options?)
decode->>decode: opts = replace(options) or new DecodeOptions()
decode->>decoder: decoder_fn = opts.decoder (adapter)
alt input is str
decode->>decode: temp_obj = _parse_query_string_values(value, opts)
else input is Mapping
decode->>decode: temp_obj = dict(value)
end
loop for each key,value in temp_obj
decode->>decoder: decoder_fn(key, charset, kind=KEY)
decoder->>utils: DecodeUtils.decode(key, charset, KEY)
decode->>decoder: decoder_fn(value, charset, kind=VALUE)
decoder->>utils: DecodeUtils.decode(value, charset, VALUE)
decode->>decode: merge pair into result (skip if key is None)
end
decode-->>Caller: return result dict
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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). (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 (
|
Coverage summary from CodacySee diff coverage on Codacy
Coverage variation details
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: Diff coverage details
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: See your quality gate settings Change summary preferences |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #23 +/- ##
==========================================
- Coverage 95.31% 93.55% -1.77%
==========================================
Files 15 16 +1
Lines 960 1070 +110
==========================================
+ Hits 915 1001 +86
- Misses 45 69 +24 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
… and parser state isolation in decoder
techouse
changed the title
![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
🔭 Outside diff range comments (1)
src/qs_codec/decode.py (1)
252-255: Bug: Array values are coerced to a comma-joined string when interpreting numeric entitiesThis loses list semantics and differs from how values are decoded (where Utils.apply preserves arrays). Interpret numeric entities element-wise instead.
- if val and options.interpret_numeric_entities and charset == Charset.LATIN1: - val = _interpret_numeric_entities( - val if isinstance(val, str) else ",".join(val) if isinstance(val, (list, tuple)) else str(val) - ) + if val and options.interpret_numeric_entities and charset == Charset.LATIN1: + val = Utils.apply(val, lambda s: _interpret_numeric_entities(s) if isinstance(s, str) else s)
🧹 Nitpick comments (7)
src/qs_codec/enums/decode_kind.py (1)
10-10: Optional: use Enum.auto() to avoid hard-coded values.This prevents accidental value coupling and simplifies maintenance without changing behavior.
-from enum import Enum +from enum import Enum, auto @@ - KEY = 1 # Key/segment decode; preserve encoded dots for later splitting logic - VALUE = 2 # Value decode; fully percent-decode + KEY = auto() # Key/segment decode; preserve encoded dots for later splitting logic + VALUE = auto() # Value decode; fully percent-decodeAlso applies to: 26-27
src/qs_codec/__init__.py (1)
17-17: Consider bumping version for the new public enum.Adding a new public symbol (DecodeKind) typically warrants a minor version bump. If you plan to cut a release from this commit, consider 1.2.0.
-__version__ = "1.1.8" +__version__ = "1.2.0"tests/unit/decode_test.py (1)
10-10: Prefer importing DecodeKind via the public package root.Since you re-exported DecodeKind, import it as a first-class public API.
-from qs_codec.enums.decode_kind import DecodeKind +from qs_codec import DecodeKindsrc/qs_codec/models/decode_options.py (2)
91-101: Docstring mentions a non-existent DecodeError.The current behavior raises IndexError (strict_depth) and ValueError (other limits in tests). Consider aligning the doc to actual exceptions or introducing a concrete DecodeError if that’s the intended API.
- When ``True``, the decoder will not descend beyond ``depth`` levels. Combined with - ``raise_on_limit_exceeded``: - - - if ``raise_on_limit_exceeded=True``, exceeding the depth results in a - ``DecodeError.depth_exceeded``; - - if ``False``, the decoder stops descending and treats deeper content as a terminal - value, preserving the last valid container without raising. + When ``True``, the decoder will not descend beyond ``depth`` levels. Combined with + ``raise_on_limit_exceeded``: + + - if ``raise_on_limit_exceeded=True``, exceeding the depth raises an ``IndexError``; + - if ``False``, the decoder stops descending and treats deeper content as a terminal + value, preserving the last valid container without raising.
120-127: Decoder docstring is mostly accurate; minor clarity tweak.You may clarify that a custom decoder returning None results in None being used as the scalar value, which affects strict_null_handling cases.
- If a custom decoder does not accept ``kind``, it will be automatically adapted so existing - decoders continue to work. + If a custom decoder does not accept ``kind``, it will be automatically adapted so existing + decoders continue to work. Returning ``None`` from the decoder uses ``None`` as the scalar value.src/qs_codec/decode.py (2)
66-68: Tighten the error message for type validation (nit)The current message is a bit generic and uses “String” (Java-ism). A clearer, Pythonic message helps users.
- raise ValueError("The input must be a String or a Mapping") + raise ValueError("value must be a str or a Mapping[str, Any]")
72-81: parse_lists toggle happens too late to affect tokenization path (optional)The temporary parse_lists toggle is computed after calling _parse_query_string_values, so it doesn’t influence that phase. If the intent is to avoid quadratic work for large flat query strings, toggling before tokenization ensures both tokenization and folding honor the same setting. You can pre-count parameters cheaply and toggle before parsing.
- temp_obj: t.Optional[t.Dict[str, t.Any]] = ( - _parse_query_string_values(value, opts) if isinstance(value, str) else dict(value) - ) - - # Temporarily toggle parse_lists for THIS call only, and only for raw strings - orig_parse_lists = opts.parse_lists - try: - if isinstance(value, str) and temp_obj is not None and orig_parse_lists and 0 < opts.list_limit < len(temp_obj): - opts.parse_lists = False + # Temporarily toggle parse_lists for THIS call only, and only for raw strings + orig_parse_lists = opts.parse_lists + try: + if isinstance(value, str) and orig_parse_lists: + # Pre-count parameters so we can decide on toggling before tokenization/decoding + _s = value.replace("?", "", 1) if opts.ignore_query_prefix else value + if isinstance(opts.delimiter, re.Pattern): + _parts_count = len(re.split(opts.delimiter, _s)) if _s else 0 + else: + _parts_count = (_s.count(opts.delimiter) + 1) if _s else 0 + if 0 < opts.list_limit < _parts_count: + opts.parse_lists = False + temp_obj: t.Optional[t.Dict[str, t.Any]] = ( + _parse_query_string_values(value, opts) if isinstance(value, str) else dict(value) + )
📜 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 (6)
src/qs_codec/__init__.py(2 hunks)src/qs_codec/decode.py(8 hunks)src/qs_codec/enums/decode_kind.py(1 hunks)src/qs_codec/models/decode_options.py(4 hunks)src/qs_codec/utils/decode_utils.py(4 hunks)tests/unit/decode_test.py(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (5)
tests/unit/decode_test.py (5)
src/qs_codec/enums/decode_kind.py (1)
DecodeKind(13-27)src/qs_codec/enums/charset.py (1)
Charset(25-36)src/qs_codec/utils/decode_utils.py (2)
DecodeUtils(17-186)decode(68-114)src/qs_codec/decode.py (1)
decode(31-95)src/qs_codec/models/decode_options.py (1)
DecodeOptions(13-165)
src/qs_codec/__init__.py (1)
src/qs_codec/enums/decode_kind.py (1)
DecodeKind(13-27)
src/qs_codec/models/decode_options.py (3)
src/qs_codec/enums/charset.py (1)
Charset(25-36)src/qs_codec/enums/decode_kind.py (1)
DecodeKind(13-27)src/qs_codec/utils/decode_utils.py (2)
DecodeUtils(17-186)decode(68-114)
src/qs_codec/utils/decode_utils.py (1)
src/qs_codec/enums/decode_kind.py (1)
DecodeKind(13-27)
src/qs_codec/decode.py (5)
src/qs_codec/enums/charset.py (1)
Charset(25-36)src/qs_codec/enums/decode_kind.py (1)
DecodeKind(13-27)src/qs_codec/models/decode_options.py (1)
DecodeOptions(13-165)src/qs_codec/utils/utils.py (3)
Utils(31-382)merge(40-190)apply(340-350)src/qs_codec/utils/decode_utils.py (2)
DecodeUtils(17-186)decode(68-114)
⏰ 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). (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (15)
src/qs_codec/enums/decode_kind.py (1)
13-28: Well-scoped enum and clear docs. LGTM.Enum members and the docstring convey the key/value decoding distinction crisply, and the module-level all export is correct.
src/qs_codec/__init__.py (1)
22-22: Re-exporting DecodeKind at the package root looks good.Import and all update are consistent with the public API surface.
Also applies to: 40-40
tests/unit/decode_test.py (2)
1359-1416: Great coverage for kind-aware decoder behavior.These tests validate KEY/VALUE call order, percent-encoded dot handling, legacy single/two-arg decoders, and keyword-only kind. Solid.
1418-1433: State isolation test is on point.Good verification that parse_lists toggling does not leak across decode() calls via the options object.
src/qs_codec/utils/decode_utils.py (4)
68-74: Signature and doc updates for kind-aware decoding look correct.The default VALUE kind and Optional[str] return type align with usage across the codebase.
93-109: Key/value branching for Latin-1 is implemented correctly.
- KEY preserves %2E/%2e by emitting them verbatim while decoding other %XX bytes.
- VALUE decodes all %XX bytes.
The fast path and minimal allocations are appreciated.
111-114: UTF‑8 protection step for keys is correct and minimal.Pre-protecting %2E/%2e avoids premature unquoting to '.', preserving later dot semantics. The "%2" precheck avoids unnecessary work.
134-136: Robust backreference in DOT_TO_BRACKET substitution.Using \g<1> avoids ambiguity and is safer than \1 in Python’s re module.
src/qs_codec/models/decode_options.py (1)
16-23: allow_dots/decode_dot_in_keys defaults are reasonable and consistent.Defaulting decode_dot_in_keys to False and mirroring to allow_dots when unspecified is sound. The consistency guard (decode_dot_in_keys implies allow_dots) is appropriate.
src/qs_codec/decode.py (6)
31-34: Public API updates to decode() look goodAccepting Mapping input and Optional[DecodeOptions] with a safe default improves flexibility without breaking the alias contract. No issues spotted here.
178-201: Parameter splitting logic is solid; good use of maxsplit to detect overflowThe handling for regex vs. string delimiters, and the raise-on-overflow branch looks correct and mirrors Python’s split semantics. Good job preserving strict-mode behavior.
219-221: Decoder indirection is correct and kind-awarePulling a non-optional decoder_fn and passing DecodeKind improves clarity and keeps adapter logic in DecodeOptions effective.
231-243: Key decoding and None-skip semantics are correctUsing KEY context for keys and skipping None aligns with the new contract and avoids building invalid paths.
369-374: Explicit bool cast for allow_dots is appropriatesplit_key_into_segments expects a bool. This cast avoids Optional pitfalls and is consistent with DecodeOptions’ post-init rules.
69-71: Confirmed: DecodeOptions is a dataclass –replace()is safe
No changes needed here.
…, and **kwargs for charset and kind parameters
…ting parameters before parsing
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/qs_codec/models/decode_options.py (1)
148-168: Thanks for switching to signature introspection.Moving away from exception-driven probing addresses the prior concern about accidentally swallowing user
TypeErrors. This is a safer foundation for adapter logic.
🧹 Nitpick comments (3)
src/qs_codec/enums/decode_kind.py (2)
13-27: Prefer a str-backed Enum for ergonomics and JSON/logging compatibility.Sub-classing
strhelps with:
- Natural comparisons against
"key"/"value".- Serialization in contexts that expect strings (e.g., JSON, logs).
Proposed change:
-class DecodeKind(Enum): +class DecodeKind(str, Enum):
14-24: Nit: unify wording (“query string” vs “querystring”).Minor consistency fix in the class docstring.
- """Decoding context for querystring tokens. + """Decoding context for query string tokens.src/qs_codec/models/decode_options.py (1)
146-218: Avoid per-call signature introspection; precompute dispatch once and broaden compatibility.
- Performance:
inspect.signature()runs on every decode call, which can be hot (per token). Precompute once in__post_init__.- Compatibility: consider
*args/**kwargsacceptance forcharset/kind, and treatkindannotation “string-like” (e.g.,str,Optional[str],Union[..., str],Literal["key","value"]) as a cue to passkind.value.Proposed refactor (precompute dispatch; preserve the no-swallowing-user-exceptions property):
else: user_dec = self.decoder - - def _adapter( - s: t.Optional[str], - charset: t.Optional[Charset] = Charset.UTF8, - *, - kind: DecodeKind = DecodeKind.VALUE, - ) -> t.Optional[str]: - """Adapter that dispatches based on the user decoder's signature. - - Supported forms: - - dec(s) - - dec(s, charset) - - dec(s, charset, kind) - - dec(s, charset, *, kind=...) - If **kwargs is present, we may pass `kind` as a keyword even without an explicit parameter. - """ - try: - sig = inspect.signature(user_dec) - except (TypeError, ValueError): - # Builtins/callables without a retrievable signature: conservative call - return user_dec(s, charset) - - params = sig.parameters - param_list = list(params.values()) - - # Does it accept **kwargs? - has_var_kw = any(p.kind == inspect.Parameter.VAR_KEYWORD for p in param_list) - - # Charset handling - accepts_charset_pos = False - accepts_charset_kw = False - if "charset" in params: - p = params["charset"] - if p.kind in (inspect.Parameter.POSITIONAL_ONLY, inspect.Parameter.POSITIONAL_OR_KEYWORD): - accepts_charset_pos = True - if p.kind in (inspect.Parameter.POSITIONAL_OR_KEYWORD, inspect.Parameter.KEYWORD_ONLY): - accepts_charset_kw = True - - # Kind handling - has_kind_param = "kind" in params - accepts_kind_kw = False - if has_kind_param: - k = params["kind"] - accepts_kind_kw = k.kind in ( - inspect.Parameter.POSITIONAL_OR_KEYWORD, - inspect.Parameter.KEYWORD_ONLY, - ) - elif has_var_kw: - accepts_kind_kw = True # can pass via **kwargs - - # Choose representation for `kind` based on annotation, default to enum - kind_arg: t.Union[DecodeKind, str] = kind - if has_kind_param: - ann = params["kind"].annotation - if ann is str or getattr(ann, "__name__", None) == "str": - kind_arg = kind.value - - # Build call - args: t.List[t.Any] = [s] - kwargs: t.Dict[str, t.Any] = {} - - if accepts_charset_pos: - args.append(charset) - elif accepts_charset_kw: - kwargs["charset"] = charset - - if accepts_kind_kw: - kwargs["kind"] = kind_arg - - return user_dec(*args, **kwargs) - - self.decoder = _adapter + # Precompute signature details once for performance and compatibility + try: + sig = inspect.signature(user_dec) + except (TypeError, ValueError): + # Builtins/callables without a retrievable signature: conservative adapter (no kind) + def _adapter( + s: t.Optional[str], + charset: t.Optional[Charset] = Charset.UTF8, + *, + kind: DecodeKind = DecodeKind.VALUE, + ) -> t.Optional[str]: + return user_dec(s, charset) + + self.decoder = _adapter + else: + params = sig.parameters + param_list = list(params.values()) + + has_var_kw = any(p.kind is inspect.Parameter.VAR_KEYWORD for p in param_list) + has_var_pos = any(p.kind is inspect.Parameter.VAR_POSITIONAL for p in param_list) + + # Charset acceptance + accepts_charset_pos = False + accepts_charset_kw = False + if "charset" in params: + p = params["charset"] + if p.kind in (inspect.Parameter.POSITIONAL_ONLY, inspect.Parameter.POSITIONAL_OR_KEYWORD): + accepts_charset_pos = True + if p.kind in (inspect.Parameter.POSITIONAL_OR_KEYWORD, inspect.Parameter.KEYWORD_ONLY): + accepts_charset_kw = True + elif has_var_pos: + accepts_charset_pos = True # pass via positional varargs + elif has_var_kw: + accepts_charset_kw = True # pass via **kwargs + + # Kind acceptance + has_kind_param = "kind" in params + accepts_kind_kw = False + if has_kind_param: + k = params["kind"] + accepts_kind_kw = k.kind in ( + inspect.Parameter.POSITIONAL_OR_KEYWORD, + inspect.Parameter.KEYWORD_ONLY, + ) + elif has_var_kw: + accepts_kind_kw = True # pass via **kwargs + + # Should `kind` be provided as a string? + def _wants_str_kind(annotation: t.Any) -> bool: + if annotation is inspect._empty: + return False + try: + from typing import get_origin, get_args + except Exception: + return annotation is str + origin = get_origin(annotation) + if annotation is str: + return True + if origin is t.Union: + return any(_wants_str_kind(a) for a in get_args(annotation)) + if origin is t.Literal: + try: + return all(isinstance(a, str) for a in get_args(annotation)) + except Exception: + return False + return False + + force_str_kind = _wants_str_kind(params["kind"].annotation) if has_kind_param else False + + def _adapter( + s: t.Optional[str], + charset: t.Optional[Charset] = Charset.UTF8, + *, + kind: DecodeKind = DecodeKind.VALUE, + ) -> t.Optional[str]: + args: t.List[t.Any] = [s] + kwargs: t.Dict[str, t.Any] = {} + if accepts_charset_pos: + args.append(charset) + elif accepts_charset_kw: + kwargs["charset"] = charset + if accepts_kind_kw: + kwargs["kind"] = kind.value if force_str_kind else kind + return user_dec(*args, **kwargs) + + self.decoder = _adapter
📜 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 (2)
src/qs_codec/enums/decode_kind.py(1 hunks)src/qs_codec/models/decode_options.py(4 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/qs_codec/models/decode_options.py (1)
src/qs_codec/enums/decode_kind.py (1)
DecodeKind(13-27)
🔇 Additional comments (3)
src/qs_codec/enums/decode_kind.py (1)
1-8: Good addition: clear separation of key vs. value decoding context.The docstring crisply explains the preservation of percent-encoded dots for keys. This will help keep dot-splitting semantics consistent with parser options.
src/qs_codec/models/decode_options.py (2)
17-24: Option defaults and consistency check look correct.
- Defaulting
decode_dot_in_keystoFalseand derivingallow_dotswhen unspecified is sensible.- Enforcing
decode_dot_in_keys=Trueimpliesallow_dots=Truevia a runtime check aligns with the docs and prevents misconfiguration.Also applies to: 133-141
121-128: Decoder docstring clearly communicates the new preferred signature.Good clarity around the preferred
(s, charset, kind=...)form and compatibility behavior for legacy decoders.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/qs_codec/models/decode_options.py (2)
3-3: Switch to signature inspection acknowledged; good move.Importing inspect to drive kind/charset dispatch addresses the prior concern about exception-driven probing.
146-216: Adapter is robust; propose minor compatibility improvements.Great switch to inspect.signature-based dispatch (addresses earlier feedback). Two optional refinements to improve compatibility with flexible decoders:
- Treat varargs/kwargs as acceptance of charset/kind and pass them accordingly.
- Default the kind representation to string when the annotation is unknown (safer for legacy decoders that compare to "key"/"value" without annotations).
Apply this diff to the adapter:
user_dec = self.decoder def _adapter( s: t.Optional[str], charset: t.Optional[Charset] = Charset.UTF8, *, kind: DecodeKind = DecodeKind.VALUE, ) -> t.Optional[str]: """Adapter that dispatches based on the user decoder's signature. Supported forms: - dec(s) - dec(s, charset) - dec(s, charset, kind) - dec(s, charset, *, kind=...) If **kwargs is present, we may pass `kind` as a keyword even without an explicit parameter. """ try: sig = inspect.signature(user_dec) except (TypeError, ValueError): # Builtins/callables without a retrievable signature: conservative call return user_dec(s, charset) params = sig.parameters param_list = list(params.values()) # Does it accept **kwargs? has_var_kw = any(p.kind == inspect.Parameter.VAR_KEYWORD for p in param_list) + # Does it accept *args? + has_var_pos = any(p.kind == inspect.Parameter.VAR_POSITIONAL for p in param_list) # Charset handling accepts_charset_pos = False accepts_charset_kw = False if "charset" in params: p = params["charset"] if p.kind in (inspect.Parameter.POSITIONAL_ONLY, inspect.Parameter.POSITIONAL_OR_KEYWORD): accepts_charset_pos = True if p.kind in (inspect.Parameter.POSITIONAL_OR_KEYWORD, inspect.Parameter.KEYWORD_ONLY): accepts_charset_kw = True + # If *args is present, we can safely pass charset positionally. + if has_var_pos: + accepts_charset_pos = True # Kind handling has_kind_param = "kind" in params accepts_kind_kw = False if has_kind_param: k = params["kind"] accepts_kind_kw = k.kind in ( inspect.Parameter.POSITIONAL_OR_KEYWORD, inspect.Parameter.KEYWORD_ONLY, ) elif has_var_kw: accepts_kind_kw = True # can pass via **kwargs - # Choose representation for `kind` based on annotation, default to enum - kind_arg: t.Union[DecodeKind, str] = kind - if has_kind_param: - ann = params["kind"].annotation - if ann is str or getattr(ann, "__name__", None) == "str": - kind_arg = kind.value + # Choose representation for `kind`; default to string for broader compatibility when uncertain. + kind_arg: t.Union[DecodeKind, str] = kind.value + if has_kind_param: + ann = params["kind"].annotation + # Prefer enum when explicitly annotated as DecodeKind + if ann is DecodeKind or getattr(ann, "__name__", None) == "DecodeKind": + kind_arg = kind # Build call args: t.List[t.Any] = [s] kwargs: t.Dict[str, t.Any] = {} - if accepts_charset_pos: + if accepts_charset_pos: args.append(charset) - elif accepts_charset_kw: + elif accepts_charset_kw or has_var_kw: kwargs["charset"] = charset if accepts_kind_kw: kwargs["kind"] = kind_arg return user_dec(*args, **kwargs)Optional polish: preserve user_dec’s metadata on the adapter.
# At definition time from functools import wraps @wraps(user_dec) def _adapter(...): ...
🧹 Nitpick comments (1)
src/qs_codec/models/decode_options.py (1)
119-126: Decoder typing and doc updates are appropriate.
- Relaxing to Callable[..., Optional[str]] avoids mypy friction for custom decoders.
- The doc correctly signals the preferred form with kind while keeping legacy signatures viable.
Minor: consider adding a Decoder type alias to centralize this signature in one place if used elsewhere.
📜 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 (2)
src/qs_codec/decode.py(8 hunks)src/qs_codec/models/decode_options.py(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/qs_codec/decode.py
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/qs_codec/models/decode_options.py (4)
src/qs_codec/enums/charset.py (1)
Charset(25-36)src/qs_codec/enums/decode_kind.py (1)
DecodeKind(13-27)src/qs_codec/utils/decode_utils.py (2)
DecodeUtils(17-186)decode(68-114)src/qs_codec/decode.py (1)
decode(31-101)
⏰ 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). (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (4)
src/qs_codec/models/decode_options.py (4)
17-23: Option defaults and invariants are consistent.Making allow_dots/decode_dot_in_keys optional with post_init normalization, plus enforcing decode_dot_in_keys=True implies allow_dots=True, matches the docs and prevents ambiguous states.
98-100: Doc clarification for strict_depth behavior looks good.The consequences under raise_on_limit_exceeded are now explicit and unambiguous.
131-138: Normalization of allow_dots/decode_dot_in_keys is correct.Defaults are set deterministically and the invariant check prevents misconfiguration early.
141-143: Sensible defaulting of decoder to DecodeUtils.decode.Keeps behavior stable when users don’t supply a decoder.
…ve kind argument selection for decoder compatibility
…n and metadata preservation
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/qs_codec/enums/duplicates.py (1)
27-34: No.valueorint/strcasts found—auto()is safe internallyA project-wide scan shows no uses of
Duplicates.valueor casting the enum toint/str. All comparisons use enum identity (options.duplicates == Duplicates.COMBINE), so the numeric assignments fromauto()are not relied on within this codebase.Optional future hardening (only if you expose
.valueexternally or persist numeric values):• Lock numeric values
--- a/src/qs_codec/enums/duplicates.py +++ b/src/qs_codec/enums/duplicates.py class Duplicates(Enum): - COMBINE = auto() + COMBINE = 1 - FIRST = auto() + FIRST = 2 - LAST = auto() + LAST = 3• Switch to string-valued enum
(also update class definition toclass Duplicates(str, Enum):)--- a/src/qs_codec/enums/duplicates.py +++ b/src/qs_codec/enums/duplicates.py class Duplicates(Enum): - COMBINE = auto() + COMBINE = "combine" - FIRST = auto() + FIRST = "first" - LAST = auto() + LAST = "last"No mandatory changes needed.
src/qs_codec/models/decode_options.py (2)
18-25: Optional flags and invariant enforcement LGTM; minor doc wording nitMaking allow_dots and decode_dot_in_keys Optional and deriving allow_dots from decode_dot_in_keys improves configuration clarity. The invariant check is correct.
Nit: clarify that “dots in keys” refers to percent-encoded dots to avoid confusion with literal dots.
Apply this diff to tighten the wording:
- """Set to ``True`` to decode dots in keys. + """Set to ``True`` to decode percent‑encoded dots in keys (e.g., ``%2E`` → ``.``).
141-224: Avoid per-call introspection; precompute dispatch onceThe introspection-based adapter is the right direction and avoids the earlier TypeError-probing pitfall. However, calling inspect.signature and recomputing flags on every decode incurs avoidable overhead on hot paths. Precompute the dispatch once during post_init (or lazily on first call) and reuse it.
Also, when inspect.signature fails (e.g., builtins), the current fallback calls user_dec(s, charset) unconditionally, which may break decoders that only accept a single argument. Prefer a more compatible fallback: try dec(s) first, then dec(s, charset) only for arity mismatches.
Apply this refactor to compute the dispatch once and improve the fallback:
else: user_dec = self.decoder - - @wraps(user_dec) - def _adapter( - s: t.Optional[str], - charset: t.Optional[Charset] = Charset.UTF8, - *, - kind: DecodeKind = DecodeKind.VALUE, - ) -> t.Optional[str]: - """Adapter that dispatches based on the user decoder's signature. - - Supported forms: - - dec(s) - - dec(s, charset) - - dec(s, charset, kind) - - dec(s, charset, *, kind=...) - If **kwargs is present, we may pass `kind` as a keyword even without an explicit parameter. - """ - try: - sig = inspect.signature(user_dec) - except (TypeError, ValueError): - # Builtins/callables without a retrievable signature: conservative call - return user_dec(s, charset) - - params = sig.parameters - param_list = list(params.values()) - - # Does it accept **kwargs? - has_var_kw = any(p.kind == inspect.Parameter.VAR_KEYWORD for p in param_list) - # Does it accept *args? - has_var_pos = any(p.kind == inspect.Parameter.VAR_POSITIONAL for p in param_list) - - # Charset handling - accepts_charset_pos = False - accepts_charset_kw = False - if "charset" in params: - p = params["charset"] - if p.kind in (inspect.Parameter.POSITIONAL_ONLY, inspect.Parameter.POSITIONAL_OR_KEYWORD): - accepts_charset_pos = True - if p.kind in (inspect.Parameter.POSITIONAL_OR_KEYWORD, inspect.Parameter.KEYWORD_ONLY): - accepts_charset_kw = True - # If *args is present, we can safely pass charset positionally. - if has_var_pos: - accepts_charset_pos = True - - # Kind handling - has_kind_param = "kind" in params - accepts_kind_kw = False - if has_kind_param: - k = params["kind"] - accepts_kind_kw = k.kind in ( - inspect.Parameter.POSITIONAL_OR_KEYWORD, - inspect.Parameter.KEYWORD_ONLY, - ) - elif has_var_kw: - accepts_kind_kw = True # can pass via **kwargs - - # Choose representation for `kind`; default to string for broader compatibility when uncertain. - kind_arg: t.Union[DecodeKind, str] = kind.value - if has_kind_param: - ann = params["kind"].annotation - # Prefer enum when explicitly annotated as DecodeKind - if ann is DecodeKind or getattr(ann, "__name__", None) == "DecodeKind": - kind_arg = kind - - # Build call - args: t.List[t.Any] = [s] - kwargs: t.Dict[str, t.Any] = {} - - if accepts_charset_pos: - args.append(charset) - elif accepts_charset_kw or has_var_kw: - kwargs["charset"] = charset - - if accepts_kind_kw: - kwargs["kind"] = kind_arg - - return user_dec(*args, **kwargs) - - self.decoder = _adapter + # Precompute dispatch to avoid per-call introspection. + try: + sig = inspect.signature(user_dec) + params = sig.parameters + param_list = list(params.values()) + + has_var_kw = any(p.kind == inspect.Parameter.VAR_KEYWORD for p in param_list) + has_var_pos = any(p.kind == inspect.Parameter.VAR_POSITIONAL for p in param_list) + + accepts_charset_pos = False + accepts_charset_kw = False + if "charset" in params: + p = params["charset"] + if p.kind in (inspect.Parameter.POSITIONAL_ONLY, inspect.Parameter.POSITIONAL_OR_KEYWORD): + accepts_charset_pos = True + if p.kind in (inspect.Parameter.POSITIONAL_OR_KEYWORD, inspect.Parameter.KEYWORD_ONLY): + accepts_charset_kw = True + if has_var_pos: + accepts_charset_pos = True + + has_kind_param = "kind" in params + accepts_kind_kw = False + if has_kind_param: + k = params["kind"] + accepts_kind_kw = k.kind in ( + inspect.Parameter.POSITIONAL_OR_KEYWORD, + inspect.Parameter.KEYWORD_ONLY, + ) + elif has_var_kw: + accepts_kind_kw = True # can pass via **kwargs + + prefer_enum_kind = False + if has_kind_param: + ann = params["kind"].annotation + prefer_enum_kind = ann is DecodeKind or getattr(ann, "__name__", None) == "DecodeKind" + + def dispatch( + s: t.Optional[str], + charset: t.Optional[Charset], + kind: DecodeKind, + ) -> t.Optional[str]: + kind_arg: t.Union[DecodeKind, str] = kind if prefer_enum_kind else kind.value + args: t.List[t.Any] = [s] + kwargs: t.Dict[str, t.Any] = {} + if accepts_charset_pos: + args.append(charset) + elif accepts_charset_kw or has_var_kw: + kwargs["charset"] = charset + if accepts_kind_kw: + kwargs["kind"] = kind_arg + return user_dec(*args, **kwargs) + except (TypeError, ValueError): + # Builtins/callables without retrievable signature: try the most compatible forms. + def dispatch( + s: t.Optional[str], + charset: t.Optional[Charset], + kind: DecodeKind, + ) -> t.Optional[str]: + try: + return user_dec(s) # type: ignore[misc] + except TypeError: + return user_dec(s, charset) # type: ignore[misc] + + @wraps(user_dec) + def _adapter( + s: t.Optional[str], + charset: t.Optional[Charset] = Charset.UTF8, + *, + kind: DecodeKind = DecodeKind.VALUE, + ) -> t.Optional[str]: + """Adapter that dispatches based on the user decoder's signature.""" + return dispatch(s, charset, kind) + + self.decoder = _adapter
📜 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 (3)
src/qs_codec/enums/decode_kind.py(1 hunks)src/qs_codec/enums/duplicates.py(2 hunks)src/qs_codec/models/decode_options.py(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/qs_codec/enums/decode_kind.py
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/qs_codec/models/decode_options.py (4)
src/qs_codec/enums/charset.py (1)
Charset(25-36)src/qs_codec/enums/decode_kind.py (1)
DecodeKind(13-27)src/qs_codec/utils/decode_utils.py (2)
DecodeUtils(17-186)decode(68-114)src/qs_codec/decode.py (1)
decode(31-101)
🔇 Additional comments (3)
src/qs_codec/enums/duplicates.py (1)
8-8: Good switch toauto()import for maintainability.Importing
autois appropriate given the enum now uses auto-assigned values. No issues here.src/qs_codec/models/decode_options.py (2)
3-3: Imports and DecodeKind integration look goodBringing in inspect for signature introspection and DecodeKind for context-aware decoding is appropriate. Using functools.wraps to preserve metadata is also a good touch.
Also applies to: 5-6, 9-9
120-127: Decoder field typing and doc updates are appropriateAllowing Optional[Callable[..., Optional[str]]] and documenting the preferred
kind-aware calling convention while auto-adapting legacy decoders is a solid approach that keeps backward compatibility.
…oved clarity and compatibility
…or improved accuracy and guidance
…and improve adapter efficiency
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/qs_codec/models/decode_options.py (2)
118-125: Doc nit: clarify the “default signature” wording for the decoder.The current wording can be read as if the default decoder lacks
kind, while the defaultDecodeUtils.decodedoes support it. Consider rephrasing to emphasize that the built-in decoder supportskind, and that custom decoders omitting it are adapted.Apply this doc-only diff:
- Signature: ``Callable[[Optional[str], Optional[Charset]], Optional[str]]`` by default, but the - parser will prefer ``decoder(string, charset, kind=DecodeKind.KEY|VALUE)`` when available. - If a custom decoder does not accept ``kind``, it will be automatically adapted so existing - decoders continue to work. Returning ``None`` from the decoder uses ``None`` as the scalar value. + The built-in decoder supports ``kind`` and is invoked as + ``decoder(string, charset, kind=DecodeKind.KEY|VALUE)``. Custom decoders that omit + ``kind`` (or ``charset``) are automatically adapted for compatibility. Returning ``None`` + from the decoder uses ``None`` as the scalar value.
139-221: Signature-introspecting decoder adapter: robust and resolves prior concerns.Great move away from exception-driven probing; the precomputed dispatch via
inspect.signatureis solid and keeps userTypeErrors intact. The adapter’s handling of varargs/kwargs, keyword-onlykind, and positionalcharsetis thorough.Two small improvements:
Improve detection of
DecodeKindtype hints when wrapped inUnion/Optionalso we pass the enum instead of.valuemore often.Optional: add minimal tests to cover decoder shapes:
(s),(s, charset),(s, *, kind),(s, charset, *, kind),(s, charset, kind)and with kind annotations ofDecodeKind,Union[DecodeKind, str], and unannotated.Apply this focused diff to enhance
prefer_enum_kinddetection:- prefer_enum_kind = False - if has_kind_param: - ann = params["kind"].annotation - prefer_enum_kind = ann is DecodeKind or getattr(ann, "__name__", None) == "DecodeKind" + prefer_enum_kind = False + if has_kind_param: + ann = params["kind"].annotation + # Direct annotation to DecodeKind + prefer_enum_kind = ann is DecodeKind or getattr(ann, "__name__", None) == "DecodeKind" + # Handle Optional/Union forms like Union[DecodeKind, str] or Optional[DecodeKind] + if not prefer_enum_kind: + origin = t.get_origin(ann) + if origin is t.Union or getattr(origin, "__name__", None) == "UnionType": + prefer_enum_kind = any(a is DecodeKind for a in t.get_args(ann))If you’d like, I can add a small test matrix to validate the adapter across the decoder signature variants above.
📜 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 (1)
src/qs_codec/models/decode_options.py(5 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/qs_codec/models/decode_options.py (4)
src/qs_codec/enums/charset.py (1)
Charset(25-36)src/qs_codec/enums/decode_kind.py (1)
DecodeKind(13-27)src/qs_codec/utils/decode_utils.py (2)
DecodeUtils(17-186)decode(68-114)src/qs_codec/decode.py (1)
decode(31-101)
🔇 Additional comments (3)
src/qs_codec/models/decode_options.py (3)
3-6: Good switch to signature introspection; imports look correct.Top-level
inspectandwrapsare appropriate here, and the newDecodeKindimport aligns with the kind-aware decoding design.Also applies to: 9-9
18-24: Consistent semantics for decode_dot_in_keys imply allow_dots.Defaulting both flags to Optional and enforcing
decode_dot_in_keys=True⇒allow_dots=Truevia aValueErroris clear and prevents contradictory configurations. Mirroring the default ofallow_dotsfromdecode_dot_in_keysis also sensible.Also applies to: 135-137
99-101: Docs alignment on depth violations looks good.The text now consistently states
IndexErroris raised when exceeding depth withstrict_depth=Trueandraise_on_limit_exceeded=True. This addresses the previous inconsistency.Also applies to: 109-112
…bility with Union and Optional types
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
src/qs_codec/models/decode_options.py (6)
18-25: Clarify None-semantics of allow_dots and decode_dot_in_keys in the docsSince both options can be None and are resolved in post_init, it’s worth stating that behavior explicitly to avoid confusion for users reading the attribute docs in isolation.
Apply this diff to clarify the behavior:
- """Set to ``True`` to decode dot ``dict`` notation in the encoded input.""" + """Set to ``True`` to decode dot ``dict`` notation in the encoded input. + When ``None`` (default), it inherits the value of ``decode_dot_in_keys``.""" @@ - """Set to ``True`` to decode percent‑encoded dots in keys (e.g., ``%2E`` → ``.``). + """Set to ``True`` to decode percent‑encoded dots in keys (e.g., ``%2E`` → ``.``). Note: it implies ``allow_dots``, so ``decode`` will error if you set ``decode_dot_in_keys`` to ``True``, and - ``allow_dots`` to ``False``.""" + ``allow_dots`` to ``False``. + When ``None`` (default), it defaults to ``False``."""
127-138: Consistent defaulting and validation of dot optionsDefaulting allow_dots from decode_dot_in_keys and enforcing “decode_dot_in_keys=True implies allow_dots=True” early are both good choices. Please add a couple of unit tests to lock this down (e.g., construction with conflicting values should raise; inheritance from None should behave as documented).
If helpful, I can draft the tests for these cases.
145-175: Optional: support positional-only ‘kind’ and be explicit about positional acceptanceEdge case: a custom decoder could declare kind as positional-only (rare, but possible in Python 3.8+). Today, kind is only passed via kwargs. We can support this cheaply by detecting positional-acceptance for kind and appending it to args when keyword passing isn’t available. Not critical, but improves compatibility.
Apply this diff to add positional support for kind:
- accepts_kind_kw = False + accepts_kind_kw = False + accepts_kind_pos = False if has_kind_param: k = params["kind"] accepts_kind_kw = k.kind in ( inspect.Parameter.POSITIONAL_OR_KEYWORD, inspect.Parameter.KEYWORD_ONLY, ) + accepts_kind_pos = k.kind in ( + inspect.Parameter.POSITIONAL_ONLY, + inspect.Parameter.POSITIONAL_OR_KEYWORD, + ) elif has_var_kw: accepts_kind_kw = True # can pass via **kwargs + accepts_kind_pos = FalseAnd in the dispatch (see separate diff below) append the positional when kwargs aren’t available.
176-186: Optional: you can always pass DecodeKind (it’s a str subclass) and simplify the logicDecodeKind inherits from str, so decoders expecting a plain string will still work if they receive the enum value. This allows dropping the annotation-based detection, simplifying the dispatch.
Minimal change to simplify argument construction:
- prefer_enum_kind = False - if has_kind_param: - ann = params["kind"].annotation - # Direct annotation to DecodeKind - prefer_enum_kind = ann is DecodeKind or getattr(ann, "__name__", None) == "DecodeKind" - # Handle Optional/Union forms like Union[DecodeKind, str] or Optional[DecodeKind] - if not prefer_enum_kind: - origin = t.get_origin(ann) - if origin is t.Union or getattr(origin, "__name__", None) == "UnionType": - prefer_enum_kind = any(a is DecodeKind for a in t.get_args(ann)) + # Passing the enum is safe because it is a str subclass. + prefer_enum_kind = True @@ - kind_arg: t.Union[DecodeKind, str] = kind if prefer_enum_kind else kind.value + kind_arg: t.Union[DecodeKind, str] = kind args: t.List[t.Any] = [s] kwargs: t.Dict[str, t.Any] = {} if accepts_charset_pos: args.append(charset) elif accepts_charset_kw or has_var_kw: kwargs["charset"] = charset - if accepts_kind_kw: + if accepts_kind_kw: kwargs["kind"] = kind_arg + elif 'accepts_kind_pos' in locals() and accepts_kind_pos: + args.append(kind_arg) return user_dec(*args, **kwargs)If you prefer to keep the annotation path, consider also resolving ForwardRefs via typing.get_type_hints to improve detection accuracy.
Also applies to: 192-201
203-216: Fallback shouldn’t mask user TypeErrors; preserve the original exceptionIn the non-inspectable fallback, catching TypeError and then trying another call form can hide legitimate TypeErrors raised inside the user decoder. Re-raise the original error if both attempts fail to avoid losing the original stack trace.
Apply this diff:
def dispatch( s: t.Optional[str], charset: t.Optional[Charset], kind: DecodeKind, ) -> t.Optional[str]: # Mark `kind` as used to satisfy linters; legacy decoders ignore it. _ = kind - try: - return user_dec(s) # type: ignore[misc] - except TypeError: - return user_dec(s, charset) # type: ignore[misc] + try: + return user_dec(s) # type: ignore[misc] + except TypeError as e1: + try: + return user_dec(s, charset) # type: ignore[misc] + except TypeError: + raise e1
195-201: Complete the positional ‘kind’ support in dispatch (paired with earlier introspection tweak)If you adopt the positional-acceptance detection for kind, this finishes the dispatch path by appending kind as a positional arg when keyword passing is unavailable.
Apply this diff:
if accepts_charset_pos: args.append(charset) elif accepts_charset_kw or has_var_kw: kwargs["charset"] = charset - if accepts_kind_kw: + if accepts_kind_kw: kwargs["kind"] = kind_arg + elif 'accepts_kind_pos' in locals() and accepts_kind_pos: + args.append(kind_arg) return user_dec(*args, **kwargs)
📜 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 (1)
src/qs_codec/models/decode_options.py(5 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/qs_codec/models/decode_options.py (4)
src/qs_codec/enums/charset.py (1)
Charset(25-36)src/qs_codec/enums/decode_kind.py (1)
DecodeKind(13-27)src/qs_codec/utils/decode_utils.py (2)
DecodeUtils(17-186)decode(68-114)src/qs_codec/decode.py (1)
decode(31-101)
⏰ 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). (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (4)
src/qs_codec/models/decode_options.py (4)
3-10: Introspection-based adapter groundwork looks solidTop-level imports for inspect/wraps and DecodeKind set the stage for a safer, context-aware decoder adapter without exception-driven probing. No issues spotted here.
99-101: Depth/limit documentation is now consistent with behaviorThe docs now correctly state IndexError for depth violations when strict_depth=True and raise_on_limit_exceeded=True, and DecodeError for other limit violations. Looks good.
Also applies to: 109-111
118-126: Decoder type widening and documentation look correctBroadening the type to Callable[..., Optional[str]] and documenting the preferred call form with kind is appropriate and keeps backward compatibility.
217-227: Adapter wrapper is lightweight and preserves metadataUsing @wraps to keep name/doc/signature metadata while standardizing the call-site is a nice touch. The cached dispatch avoids per-call introspection overhead.
…roved clarity and positional argument support
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/qs_codec/models/decode_options.py (1)
101-103: Docs are now consistent about depth violations (IndexError)This aligns with the PR summary and resolves the earlier inconsistency flagged in prior review comments.
Also applies to: 111-114
🧹 Nitpick comments (1)
src/qs_codec/models/decode_options.py (1)
153-166: Optional: broaden positional passing when varargs are presentIf a user decoder supports var-positional arguments (*args) but omits an explicit kind parameter, we currently avoid passing kind at all. Consider, as a nice-to-have, appending kind positionally when has_var_pos is True and there is no explicit kind parameter. This would improve compatibility with loosely-typed decoders that consume extra positional args.
If you want, I can draft a small unit test matrix covering custom decoder shapes:
- def d(s)
- def d(s, charset)
- def d(s, charset, *, kind)
- def d(s, charset, kind)
- def d(s, charset, kind, /) # positional-only kind
- def d(s, *args, **kwargs)
Also applies to: 167-183
📜 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 (1)
src/qs_codec/models/decode_options.py(5 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/qs_codec/models/decode_options.py (4)
src/qs_codec/enums/charset.py (1)
Charset(25-36)src/qs_codec/enums/decode_kind.py (1)
DecodeKind(13-27)src/qs_codec/utils/decode_utils.py (2)
DecodeUtils(17-186)decode(68-114)src/qs_codec/decode.py (1)
decode(31-101)
⏰ 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). (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (2)
src/qs_codec/models/decode_options.py (2)
18-26: Defaults and invariants for dot-decoding look correct
- Optional defaults with inheritance (allow_dots ← decode_dot_in_keys) are clear.
- Runtime check enforcing decode_dot_in_keys=True implies allow_dots=True is good defensive validation.
Also applies to: 131-139
147-201: Introspection-based decoder adapter is a solid improvement
- Precomputing signature-based dispatch avoids exception-driven probing and per-call overhead.
- Preserving metadata with functools.wraps is a nice touch.
- Passing DecodeKind (str subclass) ensures compatibility with decoders expecting plain str.
Also applies to: 219-229
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/qs_codec/models/decode_options.py (2)
101-103: Depth-limit docs are now consistent with strict mode behaviorDocs now correctly state IndexError for depth violations under strict_depth=True + raise_on_limit_exceeded=True, matching the PR objective.
Also applies to: 111-114
141-201: Signature-driven adapter avoids swallowing user TypeErrors; resolves prior issuesGreat improvement replacing exception-driven probing with inspect.signature-based dispatch. It:
- Correctly detects positional-only, positional-or-keyword, and keyword-only parameters for both charset and kind.
- Supports varargs/varkw, passing charset/kind appropriately.
- Fixes the earlier “locals() check” bug by correctly appending positional kind when accepted.
Two tiny nits you can consider:
- Since DecodeKind subclasses str, you don’t need the extra union annotation for kind_arg.
- kind_arg: t.Union[DecodeKind, str] = kind + kind_arg = kind # DecodeKind subclasses str; safe for decoders expecting str
- Optional: you can avoid creating param_list by iterating params.values() directly in the any(...) checks.
- param_list = list(params.values()) - - has_var_kw = any(p.kind == inspect.Parameter.VAR_KEYWORD for p in param_list) - has_var_pos = any(p.kind == inspect.Parameter.VAR_POSITIONAL for p in param_list) + has_var_kw = any(p.kind == inspect.Parameter.VAR_KEYWORD for p in params.values()) + has_var_pos = any(p.kind == inspect.Parameter.VAR_POSITIONAL for p in params.values())Overall, this section is solid and addresses earlier reviewer guidance.
🧹 Nitpick comments (1)
src/qs_codec/models/decode_options.py (1)
18-26: Option defaults and implications are correct; minor doc polishThe defaulting and implication logic between allow_dots and decode_dot_in_keys is implemented correctly. Small doc nits for clarity and style.
Apply this diff to tighten phrasing:
- Note: it implies ``allow_dots``, so ``decode`` will error if you set ``decode_dot_in_keys`` to ``True``, and - ``allow_dots`` to ``False``. - When ``None`` (default), it defaults to ``False``. + Note: it implies ``allow_dots``, so ``decode`` will error if you set ``decode_dot_in_keys=True`` and + ``allow_dots=False``. + When ``None`` (default), it evaluates to ``False``.
📜 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 (1)
src/qs_codec/models/decode_options.py(5 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/qs_codec/models/decode_options.py (4)
src/qs_codec/enums/charset.py (1)
Charset(25-36)src/qs_codec/enums/decode_kind.py (1)
DecodeKind(13-27)src/qs_codec/utils/decode_utils.py (2)
DecodeUtils(17-186)decode(68-114)src/qs_codec/decode.py (1)
decode(31-101)
⏰ 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). (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (5)
src/qs_codec/models/decode_options.py (5)
3-9: Imports and enum integration LGTMBringing in inspect, wraps, and DecodeKind is aligned with the new kind-aware decoding flow and the adapter. No concerns.
120-127: Decoder API docs reflect kind-aware behaviorClear guidance on calling convention and backward-compat behavior. The Optional return type and semantics for None are well-documented. LGTM.
129-139: Defaulting logic and validation LGTM
- decode_dot_in_keys defaults to False and allow_dots mirrors it when unspecified.
- Enforcing decode_dot_in_keys=True implies allow_dots=True via ValueError is correct and prevents contradictory configurations.
No changes needed.
202-218: Conservative fallback is acceptableFor callables without an introspectable signature, the minimal (s) → (s, charset) probing keeps behavior predictable and re-raises the original TypeError. This is a reasonable compromise for builtins/C-based callables.
219-229: Adapter wrapper is clean and preserves metadata@wraps preserves user metadata; the signature and defaults align with the rest of the codebase. LGTM.
…h to support both Enum and string types
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/qs_codec/models/decode_options.py (1)
148-219: Great switch to inspect-based dispatch; resolves prior concerns.Replacing exception-driven probing with inspect.signature and precomputed dispatch is the right call. It avoids swallowing user TypeErrors and makes the adapter predictable and efficient at call time. The positional-vs-keyword handling for charset/kind and the enum-vs-string decision are thoughtfully implemented.
📜 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 (1)
src/qs_codec/models/decode_options.py(5 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/qs_codec/models/decode_options.py (3)
src/qs_codec/enums/charset.py (1)
Charset(25-36)src/qs_codec/enums/decode_kind.py (1)
DecodeKind(13-27)src/qs_codec/utils/decode_utils.py (2)
DecodeUtils(17-186)decode(68-114)
🪛 GitHub Actions: Test
src/qs_codec/models/decode_options.py
[warning] 195-195: Pylint W0718: Catching too general exception (broad-exception-caught) in src/qs_codec/models/decode_options.py:195.
⏰ 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). (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (1)
src/qs_codec/models/decode_options.py (1)
132-141: Defaults and invariants for dot-decoding are consistent and enforced.Defaulting decode_dot_in_keys to False, mirroring allow_dots when unspecified, and enforcing the implication (decode_dot_in_keys ⇒ allow_dots) match the docs and prevent misconfiguration.
… kind argument type
1 participant
This pull request introduces several improvements and refactors to the query string decoding logic, focusing on more robust handling of decoding context (key vs. value), safer option mutation, and better compatibility for custom decoders. The changes also enhance parameter limit enforcement and clarify depth handling in decoding. Below are the most important changes grouped by theme:
Decoding Context and Custom Decoder Support
DecodeKindenum to distinguish between decoding keys and values, ensuring that percent-encoded dots are preserved in keys for correct dot notation parsing. (src/qs_codec/enums/decode_kind.py)kindargument, adapting custom decoders automatically for backward compatibility. This allows users to provide decoders that are aware of whether they're decoding a key or a value. (src/qs_codec/models/decode_options.py,src/qs_codec/utils/decode_utils.py,src/qs_codec/decode.py) [1] [2] [3]Query String Decoding Logic
decodefunction to work on a local copy of options, preventing internal toggles (likeparse_lists) from leaking to the caller. Also, improved logic for toggling list parsing based on parameter count before tokenization. (src/qs_codec/decode.py)src/qs_codec/decode.py)Depth and Dot Notation Handling
IndexErrorwhen strict mode is enabled. (src/qs_codec/models/decode_options.py)src/qs_codec/utils/decode_utils.py) [1] [2]API and Typing Improvements
optionsoptional and usingMappinginstead ofdictfor input types. (src/qs_codec/decode.py,src/qs_codec/__init__.py) [1] [2]Test and Import Updates
DecodeKindand the expanded decoder interface. (tests/unit/decode_test.py,src/qs_codec/__init__.py) [1] [2]Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests