feat(logging): add structured progress output and stdlib logging - #54
Conversation
- New log.py with info/warn/skip/error helpers - Lifecycle messages in app.py: manifest loaded, discovery, processing, completed - Test lifecycle output in test_app.py
- Log module query start, resolved ref/version, and outcome (updated/ unchanged/skipped) for both github and registry modules in app.py - Migrate bare print([SKIP]) calls to log.skip() - Add tests covering start, resolution, skip and outcome log lines for both module types
- Return ModuleOutcome("updated"|"unchanged"|"skipped") from both
process functions instead of a raw int
- Aggregate outcomes in main() and emit a final [INFO] Run summary
line with modules/updated/unchanged/skipped/replacements counts
- Update tests to assert on outcome.outcome and outcome.replacements
and verify the summary line is present in lifecycle output
small-e
left a comment
small-e
left a comment
There was a problem hiding this comment.
mostly looks good - a couple of questions to address before approval though
|
|
||
| def skip(message: str) -> None: | ||
| _logger.info("[SKIP] %s", message) | ||
|
|
There was a problem hiding this comment.
ah I see where the skip is coming in. I am questioning whether this module and these functions are worthwhile since it doesn't seem there isn't anything really being added to logging (not really). Another consideration is that adding the skip caught me off guard because it strays from convention - so I would recommend not adding this module but sticking to logging conventions. I won't let it block approval if you feel strongly though.
There was a problem hiding this comment.
This was for convenience when I was refactoring but agree it's confusing and needs tidying up. I'll keep a single logging configuration point and switch modules to standard module-level loggers. For skip events, I'll emit INFO with explicit skipped outcome/reason fields for clarity.
| manifest_path, _, _, _ = manifest_data | ||
| tmp_root = manifest_path.parent | ||
|
|
||
| from update_tf_modules import discovery, targets |
There was a problem hiding this comment.
curious as to why you are importing within the test here? It seems a bit inconsistent.
There was a problem hiding this comment.
Good point. The local imports were mainly for patching convenience while I was wiring integration-style tests, but they are not strictly required - I'll move them to module scope for consistency.
2 participants
Replaces sparse
print()calls with structured, levelled log output across the module update workflow.Closes #27
Changes
loggingmodule (LOG_LEVELenv var,[%(levelname)s]format)main(): manifest loaded, discovery, processing, run summaryupdated/unchanged/skipped)print([SKIP])calls tolog.skip()update_github_moduleandupdate_registry_moduleto count only actual file changes, not regex matches — prevents falseupdatedoutcomes when a module is already at the latest ref/versionTests
test_app.pyfromcapsystocaplog