Implement insert mode (fixes DEV-146)#52
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
6d07e38 to
d3f34af
Compare
There was a problem hiding this comment.
Pull request overview
This PR implements an insert mode feature that allows retroactively inserting older data that was discovered later. The implementation adds a new --insert-all command-line option and makes extensive type annotation improvements throughout the codebase.
Key Changes
- Added
--insert-all <station> <filepath>CLI option to enable inserting historical data retroactively - Modified Enhydris class to support insert mode, which uses "insert" instead of "append" when posting time series data
- Refactored MeteologgerStorage to accept
configparser.SectionProxyinstead of plain dictionaries - Added comprehensive type annotations using Python 3.10+ style (e.g.,
dict[str, Any]instead ofDict[str, Any])
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| loggertodb/cli.py | Added --insert-all option, refactored configuration reading to support single-station processing with custom file path, added type annotations |
| loggertodb/enhydris.py | Added use_insert_mode parameter, modified _get_ts_end_dates to skip fetching end dates in insert mode, added mode parameter to post_tsdata call, converted to NamedTuple |
| loggertodb/meteologgerstorage.py | Changed parameter type from dict to configparser.SectionProxy, added extensive type annotations, refactored error handling structure, added type safety improvements |
| tests/test_cli.py | Added tests for insert-all functionality, refactored existing tests to use better mock parameter naming, added type annotations |
| tests/test_enhydris.py | Added test_insert_all to verify insert mode behavior, updated constructor calls to include use_insert_mode parameter, added type annotations |
| tests/loggerstorage/test_loggerstorage.py | Updated tests to use configparser.SectionProxy instead of dicts, added type annotations |
| tests/loggerstorage/test_multitextfileloggerstorage.py | Updated tests to use configparser.SectionProxy, added timezone directory handling for pyfakefs, added type annotations |
| docs/usage.rst | Added command-line reference section documenting the --insert-all option with usage examples and limitations |
| pyproject.toml | Updated pthelma dependency to >=2.8.1 and pinned pyfakefs to 5.3.x range |
Comments suppressed due to low confidence (1)
loggertodb/meteologgerstorage.py:909
- The variable
vis initialized to an empty string, but ifself.variablesis empty or no matchingts_idis found in the loop,vwill remain an empty string and be used as a dictionary key on line 909, which would likely cause a KeyError. Consider adding an assertion after the loop similar to the pattern used inTextFileMeteologgerStorage._extract_value_and_flags(line 280):
assert v != ""or
assert isinstance(v, str) and v v = ""
for v, tid in self.variables.items():
if tid == ts_id:
break
return (record[v], "")
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #52 +/- ##
==========================================
+ Coverage 89.10% 89.34% +0.24%
==========================================
Files 6 6
Lines 789 826 +37
==========================================
+ Hits 703 738 +35
- Misses 86 88 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
d3f34af to
ef675d5
Compare
ef675d5 to
3baeea6
Compare
No description provided.