Release 0.5.0: jitter, refresh opt-out, perf improvements#12
Merged
Conversation
- ruff: enable B, I, UP, RUF rule sets; set line-length=120; ignore E501 in tests - pyright: upgrade basic → standard; disable override/private rules; enable reportUnnecessaryTypeIgnoreComment
- isort: add missing blank lines between import groups, alphabetize members - pyupgrade: typing.Callable/Iterable → collections.abc, Dict → dict, timezone.utc → UTC, remove quoted annotations - remove stale noqa: D401 directive (caught by RUF100)
- B904: add 'from None' to 3 intentional exception replacements - B028: add stacklevel=2 to warnings.warn call - B009: replace getattr with constant key with direct attribute access - remove stale '# type: ignore[assignment]' (caught by reportUnnecessaryTypeIgnoreComment)
Refactor 8 resource __repr__ methods to use local variables instead of inline ternaries and getattr calls, bringing all lines under 120 chars.
Update the mutmut configuration in pyproject.toml to specify a list of directories for tests and enable `mutate_only_covered_lines`. This will improve the accuracy and efficiency of mutation testing by focusing on relevant code paths and only mutating lines that are already covered by tests. Also, update the `mut-reset` target in the Makefile to directly remove the `.mutmut-cache` directory for a cleaner reset process.
This commit introduces several new property tests to enhance the robustness of the API client library. The tests cover: - **ApiObject property handling**: Ensuring that changes are correctly tracked and that setting attributes to their existing values doesn't mark them as dirty. - **Asset custom field staging**: Verifying the correct behavior of `set_custom_field`, including staging, cancellation, and handling of unknown fields. - **BaseResourceManager list_all pagination**: Testing that `list_all` correctly handles pagination and respects the provided limit. - **Pure utility functions**: Confirming the reliability of `_parse_retry_after` and `_stringify_messages` under various inputs.
- what: replace retry transport's sleep with a no-op lambda in the shared snipeit_client fixture and test_logging's client_with_token - why: tests that exhaust retries (5xx mapping, transport-error paths) no longer incur ~2.1s real backoff delay per test - risk: none — tests needing real timing construct their own transport
- what: add _full_jitter helper and jitter= parameter to RetryTransport; backoff delays are passed through jitter(base) before sleeping - why: desynchronise retries across concurrent clients hitting the same Snipe-IT instance (thundering-herd mitigation) - risk: default behaviour changes from deterministic to randomised delays; Retry-After headers are still honoured verbatim (no jitter applied)
- what: replace copy.deepcopy with a JSON-specialised recursive copy in _safe_snapshot; cap list_all per-page request to min(page_size, remaining) and raise default page_size from 50 to 100 - why: deepcopy overhead dominated ApiObject construction on large list_all results; requesting 500 rows when only 5 remain wastes bandwidth and server resources - risk: _fast_json_copy falls back to deepcopy for non-JSON types; default page_size change is backward-compatible (server-side max still applies)
- what: add keyword-only refresh parameter (default True) to the four asset action methods; when False, skip the follow-up GET and return the same instance - why: halves round trips per action in bulk workflows where the caller does not need the updated local state - risk: none — default behaviour is unchanged; callers opting in accept stale local state until they call refresh() themselves
📝 WalkthroughWalkthroughRelease 0.5.0 introduces retry backoff jitter, optional Changesv0.5.0 Release: Features, Optimizations, and Testing
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 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)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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.
Inline comments:
In `@Makefile`:
- Around line 29-31: The recipe comment for target mut-quick is misleading
because the mut-quick Makefile target currently runs the full mutmut command
just like mut; either update the comment or change the recipe for the mut-quick
target to run a scoped mutmut invocation (e.g., use mutmut run with path/arg to
limit to highest-value files or a predefined file list/glob), and keep the mut
target unchanged; locate the Makefile target named mut-quick and either adjust
its description text to match the current full run or modify the command to a
scoped mutmut run (using the mutmut --paths-to-mutate / specific-paths approach
or similar) so behavior and comment are aligned.
In `@pyrightconfig.json`:
- Around line 8-9: Re-enable the Pyright diagnostics by setting
reportIncompatibleMethodOverride and reportIncompatibleVariableOverride back to
true in pyrightconfig.json, then fix the incompatible override in the Asset
class: change Asset.save() signature/return type so it preserves the generic
type variable T from the base class (or make the base method covariant/adjust
generics) so the override matches the base class contract; ensure any calls
using Asset.save() still type-check after adjusting the signature to satisfy
Pyright's reportIncompatibleMethodOverride check.
In `@snipeit/resources/base.py`:
- Around line 346-362: The list_all method should validate pagination inputs up
front: reject page_size <= 0 and limit < 0 with clear ValueError messages so
invalid query params are not sent to self._get; add checks at the start of
list_all to raise for these cases (reference the list_all(...) signature and the
use of page_size/limit before calling self._get) and keep existing behavior for
None/positive values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 645cf161-b507-4949-afed-605830dbcc2c
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (55)
CHANGELOG.mdMakefilepyproject.tomlpyrightconfig.jsonsnipeit/_retry.pysnipeit/client.pysnipeit/resources/accessories.pysnipeit/resources/assets/files.pysnipeit/resources/assets/manager.pysnipeit/resources/assets/model.pysnipeit/resources/base.pysnipeit/resources/categories.pysnipeit/resources/companies.pysnipeit/resources/components.pysnipeit/resources/consumables.pysnipeit/resources/departments.pysnipeit/resources/fields.pysnipeit/resources/fieldsets.pysnipeit/resources/licenses.pysnipeit/resources/locations.pysnipeit/resources/manufacturers.pysnipeit/resources/models.pysnipeit/resources/status_labels.pysnipeit/resources/suppliers.pysnipeit/resources/users.pytests/conftest.pytests/integration/conftest.pytests/integration/resources/test_accessories.pytests/integration/resources/test_assets.pytests/integration/resources/test_categories.pytests/integration/resources/test_companies.pytests/integration/resources/test_departments.pytests/integration/resources/test_fields.pytests/integration/resources/test_locations.pytests/integration/resources/test_manufacturers.pytests/integration/resources/test_models.pytests/integration/resources/test_status_labels.pytests/integration/resources/test_suppliers.pytests/integration/resources/test_users.pytests/unit/resources/test_assets.pytests/unit/resources/test_assets_labels.pytests/unit/resources/test_base.pytests/unit/resources/test_pagination.pytests/unit/resources/test_shape_validation.pytests/unit/test_assets_endpoints.pytests/unit/test_client_properties.pytests/unit/test_exceptions.pytests/unit/test_logging.pytests/unit/test_property_apiobject.pytests/unit/test_property_asset_custom_fields.pytests/unit/test_property_list_all.pytests/unit/test_property_pure_functions.pytests/unit/test_repr.pytests/unit/test_retries.pytests/unit/test_streaming_download.py
Comment on lines
29
to
+31
| # Quick mutation run scoped to the highest-value source files (used in CI) | ||
| mut-quick: | ||
| $(PY) -m mutmut run \ | ||
| --paths-to-mutate snipeit/client.py,snipeit/_retry.py,snipeit/resources/base.py \ | ||
| --tests-dir tests/unit tests/contract || true | ||
| $(PY) -m mutmut run || true |
There was a problem hiding this comment.
Update mut-quick description (or restore scoped behavior).
Line 29 says “scoped to highest-value source files”, but Line 31 runs the same full command as mut. Please align comment and behavior to avoid misleading usage.
🤖 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 `@Makefile` around lines 29 - 31, The recipe comment for target mut-quick is
misleading because the mut-quick Makefile target currently runs the full mutmut
command just like mut; either update the comment or change the recipe for the
mut-quick target to run a scoped mutmut invocation (e.g., use mutmut run with
path/arg to limit to highest-value files or a predefined file list/glob), and
keep the mut target unchanged; locate the Makefile target named mut-quick and
either adjust its description text to match the current full run or modify the
command to a scoped mutmut run (using the mutmut --paths-to-mutate /
specific-paths approach or similar) so behavior and comment are aligned.
Comment on lines
+8
to
+9
| "reportIncompatibleMethodOverride": false, | ||
| "reportIncompatibleVariableOverride": false, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Compare current pyright output vs. output with override checks enabled.
python - <<'PY'
import json, tempfile, subprocess, pathlib
cfg_path = pathlib.Path("pyrightconfig.json")
cfg = json.loads(cfg_path.read_text())
strict = dict(cfg)
strict["reportIncompatibleMethodOverride"] = True
strict["reportIncompatibleVariableOverride"] = True
tmp = tempfile.NamedTemporaryFile("w", suffix=".json", delete=False)
json.dump(strict, tmp, indent=2)
tmp.close()
print("=== Current config ===")
subprocess.run([".venv/bin/pyright", "-p", str(cfg_path)], check=False)
print("\n=== With override diagnostics enabled ===")
subprocess.run([".venv/bin/pyright", "-p", tmp.name], check=False)
PYRepository: lfctech/snipeit-python-api
Length of output: 678
🏁 Script executed:
cat -n pyrightconfig.jsonRepository: lfctech/snipeit-python-api
Length of output: 700
🏁 Script executed:
# Find all Python files in the snipeit directory
find snipeit -name "*.py" -type f | head -20Repository: lfctech/snipeit-python-api
Length of output: 649
🏁 Script executed:
wc -l snipeit/resources/base.py && cat -n snipeit/resources/base.pyRepository: lfctech/snipeit-python-api
Length of output: 19642
🏁 Script executed:
# Search for classes that inherit from ApiObject or Manager
rg "class\s+\w+\((ApiObject|Manager|BaseResourceManager)" snipeit/Repository: lfctech/snipeit-python-api
Length of output: 2378
🏁 Script executed:
# Search for __init__ method overrides in subclasses
rg "__init__" snipeit/resources/ | grep -v "base.py" | grep "def __init__"Repository: lfctech/snipeit-python-api
Length of output: 52
🏁 Script executed:
# Check if any resource subclasses override common base class methods
rg "def (save|refresh|delete|_dirty_set|mark_dirty|_apply_server_data|_get|_create|_patch)" snipeit/resources/ | grep -v "base.py"Repository: lfctech/snipeit-python-api
Length of output: 329
🏁 Script executed:
cat -n snipeit/resources/assets/model.py | head -100Repository: lfctech/snipeit-python-api
Length of output: 4587
🏁 Script executed:
wc -l snipeit/resources/assets/model.py && tail -250 snipeit/resources/assets/model.pyRepository: lfctech/snipeit-python-api
Length of output: 11388
🏁 Script executed:
# Check Asset.save() signature more carefully and compare with base
sed -n '335,340p' snipeit/resources/assets/model.py
sed -n '244,246p' snipeit/resources/base.pyRepository: lfctech/snipeit-python-api
Length of output: 407
🏁 Script executed:
# Get exact line numbers for Asset.save() signature
sed -n '335,340p' snipeit/resources/assets/model.pyRepository: lfctech/snipeit-python-api
Length of output: 321
🏁 Script executed:
# Find the exact line where save() is defined in Asset
grep -n "def save" snipeit/resources/assets/model.pyRepository: lfctech/snipeit-python-api
Length of output: 103
🏁 Script executed:
# Compare Asset.save() signature with ApiObject.save() signature
sed -n '139,145p' snipeit/resources/assets/model.py
echo "---BASE CLASS---"
sed -n '244,250p' snipeit/resources/base.pyRepository: lfctech/snipeit-python-api
Length of output: 783
🏁 Script executed:
# Search for other method overrides that might have signature differences
find snipeit/resources -name "*.py" -exec grep -l "def save\|def _apply_server_data\|def _dirty_set\|def refresh" {} \;Repository: lfctech/snipeit-python-api
Length of output: 130
🏁 Script executed:
# Check if there are any subclasses of Asset that might have their own overrides
grep -r "class.*Asset" snipeit/ | grep -v "test" | grep -v ".pyc"Repository: lfctech/snipeit-python-api
Length of output: 364
🏁 Script executed:
# Check AssetsManager for any method overrides
cat -n snipeit/resources/assets/manager.pyRepository: lfctech/snipeit-python-api
Length of output: 5014
Re-enable override compatibility diagnostics.
Disabling reportIncompatibleMethodOverride and reportIncompatibleVariableOverride hides real type safety issues. The codebase contains at least one actual incompatible method override: Asset.save() returns Asset instead of preserving the type variable T from the base class signature, which violates Liskov Substitution Principle and would be caught by these diagnostics.
🤖 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 `@pyrightconfig.json` around lines 8 - 9, Re-enable the Pyright diagnostics by
setting reportIncompatibleMethodOverride and reportIncompatibleVariableOverride
back to true in pyrightconfig.json, then fix the incompatible override in the
Asset class: change Asset.save() signature/return type so it preserves the
generic type variable T from the base class (or make the base method
covariant/adjust generics) so the override matches the base class contract;
ensure any calls using Asset.save() still type-check after adjusting the
signature to satisfy Pyright's reportIncompatibleMethodOverride check.
Comment on lines
+346
to
362
| def list_all(self, *, limit: int | None = None, page_size: int = 100, **params: Any) -> Iterable[T]: | ||
| if "offset" in params: | ||
| raise ValueError( | ||
| "Do not pass 'offset' as a filter param to list_all() — it controls " | ||
| "internal pagination and would break page iteration. " | ||
| "Use 'limit' to cap total results." | ||
| ) | ||
| page = 1 | ||
| yielded = 0 | ||
| while True: | ||
| # When the caller passes a small ``limit``, never request more rows | ||
| # from the server than we still need. ``list_all(limit=5, | ||
| # page_size=500)`` would otherwise pull 500 rows just to use 5. | ||
| remaining = None if limit is None else max(0, limit - yielded) | ||
| if remaining == 0: | ||
| return | ||
| per_page = page_size if remaining is None else min(page_size, remaining) | ||
| resp = self._get( |
There was a problem hiding this comment.
Validate pagination inputs before issuing requests.
page_size <= 0 (and limit < 0) currently falls through and can emit invalid query params, making behavior server-dependent. Add explicit guards so invalid inputs fail fast.
Suggested fix
def list_all(self, *, limit: int | None = None, page_size: int = 100, **params: Any) -> Iterable[T]:
+ if page_size <= 0:
+ raise ValueError("page_size must be > 0")
+ if limit is not None and limit < 0:
+ raise ValueError("limit must be >= 0")
if "offset" in params:
raise ValueError(
"Do not pass 'offset' as a filter param to list_all() — it controls "📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def list_all(self, *, limit: int | None = None, page_size: int = 100, **params: Any) -> Iterable[T]: | |
| if "offset" in params: | |
| raise ValueError( | |
| "Do not pass 'offset' as a filter param to list_all() — it controls " | |
| "internal pagination and would break page iteration. " | |
| "Use 'limit' to cap total results." | |
| ) | |
| page = 1 | |
| yielded = 0 | |
| while True: | |
| # When the caller passes a small ``limit``, never request more rows | |
| # from the server than we still need. ``list_all(limit=5, | |
| # page_size=500)`` would otherwise pull 500 rows just to use 5. | |
| remaining = None if limit is None else max(0, limit - yielded) | |
| if remaining == 0: | |
| return | |
| per_page = page_size if remaining is None else min(page_size, remaining) | |
| resp = self._get( | |
| def list_all(self, *, limit: int | None = None, page_size: int = 100, **params: Any) -> Iterable[T]: | |
| if page_size <= 0: | |
| raise ValueError("page_size must be > 0") | |
| if limit is not None and limit < 0: | |
| raise ValueError("limit must be >= 0") | |
| if "offset" in params: | |
| raise ValueError( | |
| "Do not pass 'offset' as a filter param to list_all() — it controls " | |
| "internal pagination and would break page iteration. " | |
| "Use 'limit' to cap total results." | |
| ) | |
| yielded = 0 | |
| while True: | |
| # When the caller passes a small ``limit``, never request more rows | |
| # from the server than we still need. ``list_all(limit=5, | |
| # page_size=500)`` would otherwise pull 500 rows just to use 5. | |
| remaining = None if limit is None else max(0, limit - yielded) | |
| if remaining == 0: | |
| return | |
| per_page = page_size if remaining is None else min(page_size, remaining) | |
| resp = self._get( |
🤖 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 `@snipeit/resources/base.py` around lines 346 - 362, The list_all method should
validate pagination inputs up front: reject page_size <= 0 and limit < 0 with
clear ValueError messages so invalid query params are not sent to self._get; add
checks at the start of list_all to raise for these cases (reference the
list_all(...) signature and the use of page_size/limit before calling self._get)
and keep existing behavior for None/positive values.
- what: patch multiprocessing.set_start_method to tolerate redundant
calls in tests/conftest.py
- why: mutmut 3.5.0 calls set_start_method('fork') at module level in
__main__.py; on Python 3.14 the context is already set when the
trampoline triggers the import, causing RuntimeError
- risk: none — the patch only suppresses the 'already set' error
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
refresh=Falseoption on checkout/checkin/audit/restore (halves round trips in bulk workflows)_fast_json_copyreplacescopy.deepcopyin snapshot tracking;list_allcaps per-page requests to remaining limitset_start_methodcrash on Python 3.14Validation