fix: honor configured framework runtime settings - #368
codeforester wants to merge 3 commits into
Conversation
| self, | ||
| standard: dict[str, Any], | ||
| dry_run: bool = False, | ||
| option_sources: Mapping[str, Any] | None = None, |
There was a problem hiding this comment.
Design risk: _create_context's new option_sources parameter defaults to None, so any caller that omits it (a subclass override, a future extension point) silently reintroduces the exact precedence bug this PR fixes — sources = option_sources or {} makes every *_source lookup resolve to None, so an explicit --no-keep-temp/--no-debug on the command line can be silently overridden by config, with no error raised. Consider making this a required keyword argument instead of defaulting to None.
| ) -> logging.Logger: | ||
| """Configure user-facing and persistent handlers for a CLI logger. | ||
|
|
||
| ``log_level`` optionally selects the user-stream threshold from DEBUG, |
There was a problem hiding this comment.
Doc/behavior mismatch: configure_logger's docstring says log_level accepts DEBUG/INFO/WARNING/ERROR/CRITICAL (uppercase), but the implementation only accepts exact lowercase strings and raises ValueError otherwise — a caller following the docstring's casing (or Python logging's own uppercase convention) gets an unexpected ValueError.
| return configured_log_file or layout.log_dir / "primary.log" | ||
|
|
||
|
|
||
| def _parameter_source_was_supplied(source: Any) -> bool: |
There was a problem hiding this comment.
Reuse: _parameter_source_was_supplied hardcodes its own set of Click ParameterSource names instead of reusing _parameter_source_rank in _lifecycle_install.py, which this module already imports and which encodes the identical precedence ordering. If Click adds a new ParameterSource variant, or _parameter_source_rank's ranking is edited, these two independently-maintained checks can silently diverge.
| elif level == "debug": | ||
| level = "info" | ||
| if _parameter_source_was_supplied(quiet_source) and quiet: | ||
| rank = { |
There was a problem hiding this comment.
Reuse: the rank dict inside _configured_stream_level duplicates logging.py's _CONFIGURED_LOG_LEVELS key-for-key. If a level is renamed/added in one copy and not the other, the quiet-clamp comparison here silently falls back to logging.INFO for the mismatched level via rank.get(level, logging.INFO) instead of erroring.
| standard.get("keep_temp") or (framework_config.keep_temp if framework_config is not None else None) | ||
| if framework_config is None or _parameter_source_was_supplied(keep_temp_source): | ||
| keep_temp = bool(standard.get("keep_temp")) | ||
| elif "keep_temp" in config_provenance or framework_config.keep_temp: |
There was a problem hiding this comment.
Cleanup: the keep_temp elif's or framework_config.keep_temp disjunct is dead code — keep_temp defaults to False and can only be True when explicitly set, which always populates config_provenance first, so this clause can never fire independently of the provenance check. Also note the if/else bodies of this branch are identical (bool(standard.get("keep_temp"))), so the three-way branch could collapse to two.
1 participant
Summary
Validation
git diff --checkpassed.Fixes #357
Fixes #358