fix: namespace log timestamp configuration - #403
codeforester wants to merge 2 commits into
Conversation
| resolved_use_utc = configured_use_utc == "1" | ||
| else: | ||
| if legacy_use_utc is not None: | ||
| warnings.warn( |
There was a problem hiding this comment.
Correctness risk: warnings.warn() for the legacy LOG_UTC env var has no exception handling anywhere in the call chain. If a consumer follows this project's own docs/migrations.md guidance and sets warnings.simplefilter("error", BaseCliDeprecationWarning) globally (e.g. in test setup), and their environment still has LOG_UTC=1 set with BASE_CLI_LOG_UTC unset, the raised warning propagates uncaught — _app_core.py only wraps the configure_logger(...) call in except OSError — crashing every CLI invocation at logger setup instead of just emitting a warning.
| else: | ||
| configured_use_utc = os.environ.get("BASE_CLI_LOG_UTC") | ||
| legacy_use_utc = os.environ.get("LOG_UTC") | ||
| if configured_use_utc is not None: |
There was a problem hiding this comment.
Correctness (silent behavior): when both BASE_CLI_LOG_UTC and LOG_UTC are set, the legacy variable is silently ignored with no deprecation warning at all, since the legacy_use_utc check is only reached in the else branch. A user mid-migration with both set gets correct behavior (new var wins) but no signal that LOG_UTC is deprecated and currently doing nothing — they may keep the stale variable indefinitely.
| if configured_use_utc is not None: | ||
| resolved_use_utc = configured_use_utc == "1" | ||
| else: | ||
| if legacy_use_utc is not None: |
There was a problem hiding this comment.
Minor: the deprecation warning fires whenever LOG_UTC is present in the environment at all, even for an inert empty-string value with no behavioral effect ("" == "1" is False, same as unset). A CI template that exports LOG_UTC= as a leftover placeholder rather than unsetting it would now emit a spurious BaseCliDeprecationWarning on every run, polluting warning-capturing test output for a variable that was never doing anything.
|
|
||
| Set `BASE_CLI_LOG_UTC=1` to make the default text formatter use UTC | ||
| timestamps. The namespaced variable takes precedence over the legacy setting. | ||
| `LOG_UTC` remains recognized during the 0.5 compatibility window, but emits a |
There was a problem hiding this comment.
Policy violation: docs/api-stability.md requires every deprecation to "remain supported for at least two minor releases and 90 calendar days, whichever is longer." Current VERSION is 0.4.3, so introducing the warning in 0.5 and scheduling removal in 0.6 is only one minor release apart — a user upgrading from 0.5.x straight to 0.6.0 loses LOG_UTC support without the two-minor-release window this project's own policy promises.
|
Two additional findings (files not touched by this diff, so not postable as inline comments):
|
This branch has not been deployed
1 participant
Fixes #394
Summary
BASE_CLI_LOG_UTCconfiguration with precedenceLOG_UTCduring the 0.5 window with a deprecation warningValidation
UV_CACHE_DIR=/private/tmp/base-cli-uv-cache uv run --extra dev pytest -q tests/test_logging.pyHosted checks are expected to run on this branch.