fix(console): don't warn on missing path dep in excluded group - #11018
Open
St4r4x wants to merge 2 commits into
Open
fix(console): don't warn on missing path dep in excluded group#11018St4r4x wants to merge 2 commits into
St4r4x wants to merge 2 commits into
Conversation
poetry-core's PathDependency.__init__ unconditionally warns if a path/directory/file dependency's target doesn't exist, at pyproject.toml parse time, before the CLI knows which --with/--without/--only groups are active for this invocation. A path dependency in an excluded group (e.g. a dev-only local package absent from a Docker image) was getting flagged even though it was never going to be installed. The already-correct, group-aware validation in Installer (dep.validate(raise_error=not op.skipped)) still runs afterward and is unaffected: a path dependency that IS needed and IS missing still fails install/sync with a real error, exactly as before. Add a suppressed_loggers mechanism to Command, letting a command raise specific loggers above WARNING for itself unless run verbose/debug. InstallCommand (and SyncCommand, which inherits from it) uses this to suppress poetry.core.packages.path_dependency's premature warning. Logger levels are global process state, so also track which logger names are currently suppressed on Application and reset any a new command doesn't ask for, so suppression from one command doesn't leak into a later command in the same process (relevant for this repo's own test suite, and for anyone using Application programmatically). Resolves: python-poetry#10461
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- When resetting previously suppressed loggers in
register_command_loggers, you currently set them to the globallevel, which can overwrite any prior per-logger configuration; consider capturing and restoring each logger’s original level (e.g., via a small mapping keyed by logger name) so that suppression is strictly temporary and does not alter existing logging configuration beyond the lifetime of the command.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- When resetting previously suppressed loggers in `register_command_loggers`, you currently set them to the global `level`, which can overwrite any prior per-logger configuration; consider capturing and restoring each logger’s original level (e.g., via a small mapping keyed by logger name) so that suppression is strictly temporary and does not alter existing logging configuration beyond the lifetime of the command.
## Individual Comments
### Comment 1
<location path="src/poetry/console/application.py" line_range="593-594" />
<code_context>
+ # Mutated in place (rather than `self._suppressed_logger_names = ...`)
+ # so the update lands on the shared ClassVar instead of shadowing it
+ # with a same-named instance attribute.
+ for name in Application._suppressed_logger_names - to_suppress:
+ logging.getLogger(name).setLevel(level)
+
+ for name in to_suppress:
</code_context>
<issue_to_address>
**issue (bug_risk):** Restoring suppressed loggers to `level` may unintentionally lower externally configured log levels.
This loop sets each previously suppressed logger to the command’s `level`, which can lower a logger that was externally configured to a stricter level (e.g., CRITICAL) down to WARNING/INFO on later, less-verbose commands. To avoid overriding stricter policies, consider either tracking which loggers were temporarily bumped to ERROR and only resetting those, or resetting with something like `max(existing_level, level)` instead of always applying `level`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+593
to
+594
| for name in Application._suppressed_logger_names - to_suppress: | ||
| logging.getLogger(name).setLevel(level) |
There was a problem hiding this comment.
issue (bug_risk): Restoring suppressed loggers to level may unintentionally lower externally configured log levels.
This loop sets each previously suppressed logger to the command’s level, which can lower a logger that was externally configured to a stricter level (e.g., CRITICAL) down to WARNING/INFO on later, less-verbose commands. To avoid overriding stricter policies, consider either tracking which loggers were temporarily bumped to ERROR and only resetting those, or resetting with something like max(existing_level, level) instead of always applying level.
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.
Resolves: #10461
Problem
PathDependency.__init__(in poetry-core) unconditionally logs a warning if a path/directory/file dependency's target doesn't exist, at the point the fullpyproject.tomlis parsed — before this CLI has determined which--with/--without/--onlygroups are actually active for the currentinstall/syncinvocation. So a path dependency in an excluded group (e.g. a dev-only local package that's never copied into a Docker image) gets flagged even though it was never going to be installed.The already-correct, group-aware validation in
Installer(installer.py:373,dep.validate(raise_error=not op.skipped)) still runs afterward and is unaffected — a path dependency that IS actually needed and IS missing still fails install/sync with a real error, exactly as before.Fix
Added a
suppressed_loggersmechanism toCommand(parallel to the existingloggerslistregister_command_loggersalready reads), letting a command raise specific loggers above WARNING for itself, unless run with--verbose/-vv/--debug.InstallCommand(andSyncCommand, which inherits from it) uses this to suppresspoetry.core.packages.path_dependency's premature warning.While testing, found and fixed a related issue: logger levels are global process state, so an earlier version of this raised the level but never reset it for a later command in the same process that doesn't ask for suppression.
Applicationnow tracks which logger names it's currently suppressing and resets any a new command doesn't ask for — relevant for anything running more than one command per process, this repo's own test suite included.Nothing changed in poetry-core; this is scoped entirely to how this CLI configures logging per command.
Tests
tests/console/test_application.py: new test covering default/--verbose/-vv.tests/console/commands/test_install.py: existing fixtures already cover the scenario. Ran the full suite deterministically and with randomized order (several times) specifically to rule out logger-state leakage across tests.Added tests for changed code.
Documentation: none needed, internal logging fix with no user-facing API change.
Note on process: I worked on this with AI assistance (Claude) to investigate the root cause, write the fix, and write/run the tests. I reviewed and understand the changes and I'm the one deciding what's in this PR.