Conversation
📝 WalkthroughWalkthroughUpdated type annotations: Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
|
@codex review |
There was a problem hiding this comment.
Pull request overview
This pull request improves type annotations and test structure across the qs_codec library. The primary change is generalizing the return type of decoder functions from str to Any, allowing custom decoders to return arbitrary types. Additionally, tests are refactored to use a helper function for consistent decoder invocation, and minor type-related improvements are made throughout the codebase.
Changes:
- Type annotations for
decoderandlegacy_decoderinDecodeOptionsnow allow returning any type instead of only strings - Test files standardized to use
require_decoder()helper function instead of direct decoder access - Configuration update to exclude
.toxdirectory from mypy checking
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/weakref_test.py | Added type annotations for nested list variables and type ignore comment for __hash__ assignment |
| tests/unit/utils_test.py | Added type ignore comments for test cases intentionally passing incorrect argument types to utility functions |
| tests/unit/example_test.py | Updated encoder/decoder test setup to use option objects directly; replaced deprecated datetime.UTC with datetime.timezone.utc |
| tests/unit/encode_test.py | Introduced options_with_encoder() helper and decimal_encoder() function to reduce test code duplication; added type cast for filter parameter |
| tests/unit/encode_options_test.py | Updated encoder identity assertions to use getattr() with fallback for safer attribute comparison |
| tests/unit/decode_test.py | Added type ignore comment for intentional invalid argument test |
| tests/unit/decode_options_test.py | Refactored all tests to use new require_decoder() helper function for consistent decoder retrieval |
| src/qs_codec/models/encode_options.py | Updated serialize_date type to allow both callable and string types |
| src/qs_codec/models/decode_options.py | Changed decoder return types from str to Any across all decoder-related signatures |
| src/qs_codec/encode.py | Updated serialize_date parameter type in _encode() function to match option model changes |
| pyproject.toml | Condensed mypy exclusion list to single line and added .tox directory |
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✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #39 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 17 17
Lines 1267 1267
=========================================
Hits 1267 1267 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Codex Review: Didn't find any major issues. Bravo. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/unit/encode_test.py (1)
102-131:⚠️ Potential issue | 🟡 MinorRemove unused f-string prefixes to satisfy Ruff/Flake8 (F541).
Six f-strings in the decimal test cases lack interpolation and should be plain strings.
🧹 Suggested fix
- pytest.param([_PI], None, f"0=3.141592653589793115997963468544185161590576171875", id="list-decimal"), + pytest.param([_PI], None, "0=3.141592653589793115997963468544185161590576171875", id="list-decimal"), @@ - f"0=3.141592653589793115997963468544185161590576171875n", + "0=3.141592653589793115997963468544185161590576171875n", @@ - pytest.param({"a": _PI}, None, f"a=3.141592653589793115997963468544185161590576171875", id="dict-decimal"), + pytest.param({"a": _PI}, None, "a=3.141592653589793115997963468544185161590576171875", id="dict-decimal"), @@ - f"a=3.141592653589793115997963468544185161590576171875n", + "a=3.141592653589793115997963468544185161590576171875n", @@ - f"a[]=3.141592653589793115997963468544185161590576171875", + "a[]=3.141592653589793115997963468544185161590576171875", @@ - f"a[]=3.141592653589793115997963468544185161590576171875n", + "a[]=3.141592653589793115997963468544185161590576171875n",
🤖 Fix all issues with AI agents
In `@tests/unit/decode_options_test.py`:
- Around line 113-121: Rename the unused parameter s in the nested test function
dec inside test_user_decoder_typeerror_is_not_swallowed to _s to follow the
Python convention for intentionally unused arguments and silence Ruff's ARG001;
locate the def dec(s: t.Optional[str]) -> t.Optional[str]: declaration and
change the parameter name to _s while leaving the body (which raises TypeError)
and the rest of the test (opts, require_decoder, pytest.raises) unchanged.
- Around line 346-349: The test decoder function dec inside
test_single_decoder_acts_like_legacy_when_ignoring_kind currently declares
unused varargs (*args, **kwargs) triggering Ruff ARG001; fix by removing the
unused parameters or renaming them to _args and **_kwargs (or otherwise using a
single underscore prefix) so they are recognized as intentionally unused; update
the dec definition accordingly to eliminate the linter warning while preserving
its behavior.
🧹 Nitpick comments (1)
tests/unit/encode_test.py (1)
30-37: Pass charset/format through toEncodeUtils.encode.
decimal_encoderignores the bound charset/format, which makes it less faithful to options.♻️ Suggested refactor
def decimal_encoder( value: t.Any, charset: t.Optional[Charset] = None, format: t.Optional[Format] = None, ) -> str: - _ = charset - _ = format - return f"{EncodeUtils.encode(value)}n" if isinstance(value, Decimal) else EncodeUtils.encode(value) + encoded = EncodeUtils.encode(value, charset=charset, format=format) + return f"{encoded}n" if isinstance(value, Decimal) else encoded
1 participant
This pull request makes several improvements to the type annotations and test structure for custom encoder/decoder hooks in the
qs_codeclibrary. The main changes are the generalization of type signatures for decoder functions to allow any return type, and a refactor of tests to consistently use a helper function for decoder invocation. There are also minor updates to configuration files.Type annotation improvements:
decoderandlegacy_decoderattributes inDecodeOptions(insrc/qs_codec/models/decode_options.py) now allow any return type (t.Optional[t.Any]), instead of being restricted tostr. This change propagates to all related function signatures and adapters. [1] [2] [3] [4] [5]serialize_dateoption in bothEncodeOptionsand the_encodefunction is updated to accept either a callable or a string, improving flexibility for date serialization. [1] [2]Test refactoring:
tests/unit/decode_options_test.pyare updated to use arequire_decoderhelper function to retrieve the decoder, instead of directly accessingopts.decoder. This standardizes test invocation and improves clarity. [1] [2] [3] [4] [5] [6] [7] [8] [9] [10] [11] [12] [13] [14] [15] [16] [17]Configuration update:
mypyconfiguration inpyproject.tomlnow excludes the.toxdirectory to prevent type checking in virtual environments.Summary by CodeRabbit
New Features
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.