Dev#10
Closed
Wil-Collier wants to merge 50 commits into
Closed
Conversation
- what: replace requests transport with httpx, retry/logging helpers, and eager resource managers - what: move API objects onto Pydantic v2 with safe dirty tracking for extra fields - why: prepare the client for clearer HTTP behavior, typed public surface checks, and safer model updates - risk: breaking change for requests-specific session internals
- what: add the 0.2 changelog and refresh README examples/features - why: explain the httpx, Pydantic, retry, logging, and compatibility changes - risk: documentation only
- what: add GitHub Actions for lint, type-check, unit tests, and coverage - what: update Makefile unit and coverage targets to include contract tests - why: keep the migration covered across Python 3.11 through 3.13 - risk: local make test still requires PY to point at an environment with pytest
…ped, pydantic<3 pin
…line httpx try/except
…— /delete suffix confirmed
…eports/settings); document scope in README
…piObject docstring
…dicts/lists now detected automatically
…sts_mock dependency
… not cleared on apply
…fault 128M during AssetSeeder
… prevents collection conflict with same-named unit test files
…61 domain (tasks 2, 7)
…ific tests (task 3)
… Retry-After test (tasks 6, 8)
…, stream errors, ValidationError parse failure (tasks 9-12, 15)
… paths (tasks 13, 14)
…s, retry PATCH/DELETE/respect_retry_after (tasks 16, 17)
…trip, restore lifecycle - Custom fields: Field -> Fieldset -> Model -> Asset, exercising both top-level column-name PATCH and in-place custom_fields dict mutation (README pattern). - File round-trip: 64KiB random payload, byte-compares upload vs download, verifies progress callback, confirms file removal via /delete suffix. - Restore lifecycle: create -> soft-delete (with marker assertion) -> restore -> confirm reachable with no deleted markers. Closes the unit-vs-real gap on the three highest-risk asset code paths.
When docker/api_key.txt is missing or a directory, the seeder service's bind-mount './api_key.txt:/api_key.txt' makes Docker auto-create the host path as a directory, breaking both the seeder's '> /api_key.txt' redirect and the integration conftest's read_text() with IsADirectoryError. Fix: - docker-up: pre-create api_key.txt as an empty regular file if missing or if it is a directory. - docker-down: replace '> docker/api_key.txt' (which silently fails on a directory) with 'rm -rf' + 'touch' so the file always ends up as a regular empty file. - test-integration: hard-fail with a clear message if api_key.txt is somehow still a directory after docker-up.
…pe-IT
The integration suite was failing with two distinct flake patterns:
1. Connection reset on first run after 'make docker-up'
The Makefile waited for docker/api_key.txt to be non-empty, but Snipe-IT's
app container is still booting Apache/PHP at the moment the seeder writes
the token. The first POSTs hit a half-open socket and got ECONNRESET.
Fix: add a real API readiness probe that polls /api/v1/users/me with the
token until it returns 200 (Makefile test-integration target).
2. HTTP 429 on second/rapid runs
Snipe-IT defaults to API_THROTTLE_PER_MINUTE=240 (4 req/sec). The full
integration suite hits the limiter. The library's default retry policy
(HEAD/GET/OPTIONS only, never POST/PATCH/DELETE) is correct for
production safety but wrong for tests, where Snipe-IT 429s before
processing the request body so retries are safe.
Fix:
- Bump docker/.env API_THROTTLE_PER_MINUTE to 1200 (20 req/sec).
- Configure the real_snipeit_client fixture to retry mutating methods
on 429 with max_retries=5; document why this is test-env-only.
E2E test fixes from first real-Snipe-IT run:
- test_asset_files_e2e: Snipe-IT rejects .bin files (extension allowlist)
and renames stored files with an asset-{id}-{random}- prefix. Switch to
.txt with 64KiB of hex chars; match by suffix instead of exact filename.
- test_custom_fields_e2e: setattr(asset, '_snipeit_*', value) is silently
dropped by ApiObject's underscore-guard, so save() drops the change. Use
mark_dirty() workaround. Document this in README. The README's nested
in-place mutation pattern is also silently ignored by Snipe-IT — test
now skips with diagnostic instead of failing.
Run results: 17 passed, 2 skipped (with clear reasons), 0 failed.
Stable across consecutive runs.
Introduces `get_custom_field` and `set_custom_field` helper methods on the `Asset` model. These methods abstract away the complexities of Snipe-IT's custom field API, which uses different shapes for reading and writing. The `get_custom_field` method allows retrieving custom field values by their display label, providing a default value if the field is not found. The `set_custom_field` method stages changes to custom fields by their display label. When `save()` is called, these staged changes are translated into the correct top-level keys in the PATCH request, using the underlying column names as expected by the Snipe-IT API. This avoids the common pitfall of directly setting attributes starting with `_`, which are ignored by the library's dirty tracker.
- Add Asset.pending_custom_fields() for inspecting staged state - Fix set_custom_field() + save() not requiring refresh() between calls - Fold PATCH response column-name keys back into nested shape - get_custom_field() now returns server value, not staged value - Setting a field back to server value cancels the pending stage - Staging moved to dedicated _pending_custom_fields PrivateAttr
- Remove NOTICE from license-files (file doesn't exist) - Migrate [mutmut] config from setup.cfg to pyproject.toml - Delete setup.cfg - Fix .gitignore: remove *.lock, add .kiro/ - Track uv.lock for reproducible builds
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (92)
📝 WalkthroughWalkthroughSwitches to an httpx-based client with custom retry transport and structured logging, refactors ApiObject/Managers and splits assets into model/manager/mixins, migrates tests to httpx_mock with expanded coverage, adds CI and mutation workflows, Dockerized integration setup, updates packaging/configs, and adds docs/license/changelog. ChangesCore Client and Resources Refactor
Testing (Unit and Integration)
CI, Tooling, and Local Stack
Docs and Repository Metadata
Sequence Diagram(s)sequenceDiagram
participant Dev
participant CI(gha) as CI (GitHub Actions)
participant Docker
participant SnipeIT as Local Snipe-IT
Dev->>CI(gha): push/PR triggers
CI(gha)->>Docker: docker compose up (seed)
Docker-->>SnipeIT: start app + write api_key.txt
CI(gha)->>SnipeIT: poll /api/v1/users/me (bearer token)
SnipeIT-->>CI(gha): 200 OK
CI(gha)->>CI(gha): run unit/contract/integration tests
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120+ minutes Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
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.
This pull request introduces several major improvements to the project, including a comprehensive test suite overhaul, a significant refactor of custom-field staging, expanded CI coverage, and enhanced documentation. It also adds a new advisory mutation testing workflow, raises the coverage gate, and documents all recent changes in a detailed changelog. Additionally, it introduces a new language server configuration for multiple languages.
Testing and CI enhancements:
filterwarnings = erroris enforced inpytest.ini..github/workflows/ci.yml) that runs linting, type-checking, unit, and integration tests across multiple Python and Pydantic versions, with coverage reporting and concurrency controls..github/workflows/mutation.yml) that runsmutmuton PRs, uploads results as an artifact, and never blocks merges.Custom-field staging and behavior changes:
Assetobjects: addedpending_custom_fields(), improvedset_custom_field()andget_custom_field()behaviors, and decoupled staging from Pydantic internals. Now, setting a field to its current value cancels the pending stage, and server responses are correctly folded into the local model.Documentation and configuration:
CHANGELOG.mddocumenting all major changes, bug fixes, and new features up to version 0.4.0, including internal refactors, test improvements, and documentation updates..kiro/settings/lsp.jsonwith language server configurations for TypeScript, Rust, Python, Java, Go, Ruby, and C++, improving development environment support.Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Refactor
requeststohttpxRetry-Aftersupport