Document and test process-wide unit-system semantics (#103)#104
Merged
Conversation
Add prominent docstring notes to the unit-aware models (DeviceStatus, DeviceFeature, ReservationEntry, WeeklyReservationEntry, and TOUPeriod) clarifying that unit-dependent computed fields read the process-wide unit-system preference at access time and share it across all async tasks and threads. Add tests locking in these semantics, and record the change in the changelog under Unreleased.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR documents and adds tests to lock in the intentional process-wide unit-system preference behavior used by the unit-aware Pydantic models, clarifying that computed fields consult nwp500.unit_system.get_unit_system() at access time and that the preference is shared across async tasks and threads.
Changes:
- Added/expanded docstring notes across unit-aware models describing the process-wide unit-system semantics and access-time evaluation.
- Added a new test module asserting that changing the global unit preference affects already-constructed instances and is observed across tasks/threads.
- Updated
CHANGELOG.rst(Unreleased) to document the decision and the new tests.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/test_unit_system_process_wide.py | Adds tests that codify process-wide (not context-local) unit-system behavior across tasks/threads. |
| src/nwp500/models/status.py | Documents that temperature computed fields read the process-wide unit preference at access time via get_unit_system(). |
| src/nwp500/models/feature.py | Documents that temperature computed fields read the process-wide unit preference at access time via get_unit_system(). |
| src/nwp500/models/schedule.py | Documents that ReservationEntry/WeeklyReservationEntry computed fields use the process-wide unit preference at access time. |
| src/nwp500/models/tou.py | Clarifies that TOU price/time computed fields are independent of the unit-system preference. |
| CHANGELOG.rst | Adds Unreleased changelog entries describing the documented semantics and new tests for #103. |
Start both threads before joining so set_ready.wait() actually exercises cross-thread synchronization instead of trivially returning because the writer thread had already finished.
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
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
Documents and adds tests locking in the deliberate process-wide unit-system preference used by unit-aware Pydantic models.
Issue #103's premise was inaccurate: it claimed the unit-dependent
computed_fieldproperties read the preference from acontextvars.ContextVar. In reality,src/nwp500/unit_system.pyuses a process-wide module-level global (_unit_system), with an explicit comment (lines 32-35) explaining that a ContextVar was rejected because MQTT message handling runs in tasks scheduled from AWS CRT callback threads whose context never inherits from the application task, causing a context-local preference to silently revert to auto-detect for all real-time data.After confirming with the repo owner, the chosen direction is to keep the process-wide global and add documentation plus tests locking in the semantics. This is non-breaking: no API changes and no major version bump.
Changes
get_unit_system()) at access time and share it across all async tasks and threads:DeviceStatus(models/status.py)DeviceFeature(models/feature.py)ReservationEntryandWeeklyReservationEntry(models/schedule.py)TOUPeriod(models/tou.py) — notes that its price/time fields are independent of the unit-system preferencetests/test_unit_system_process_wide.pyverifying that (a) setting the global preference affects already-constructed instances at access time and (b) a preference set in one async task/thread is observed by a model read in another.CHANGELOG.rstentry underUnreleased(Documentation and Added subsections).unit_system.pywas reviewed and already describes the process-wide behaviour accurately; it was left as-is.Validation
scripts/lint.py): passed (0 errors)mypy src/nwp500): passed (no issues in 49 files)pytest): 594 passed, 4 skippedCloses #103