fix: use logger instead of print() in _agent_config.py - #3966
Quratulain-bilal wants to merge 3 commits into
Conversation
Replace print(..., file=sys.stderr) with logger.warning() for consistent logging behavior. Removes unused sys import.
There was a problem hiding this comment.
Pull request overview
Replaces direct stderr warning output with module-level logging.
Changes:
- Adds a module logger.
- Replaces
print(..., file=sys.stderr)withlogger.warning(). - Removes the unused
sysimport.
Show a summary per file
| File | Description |
|---|---|
src/specify_cli/_agent_config.py |
Routes invalid integration warnings through logging. |
Review details
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Balanced
mnriem
left a comment
mnriem
left a comment
There was a problem hiding this comment.
Address Copilot feedback and fix test & lint errors
Replace print(..., file=sys.stderr) with logger.warning() for consistent logging behavior. Adds logging import and logger instance. Removes unused sys import. Adds regression test for invalid integration warning. Closes github#3966
Verifies that invalid integration warnings use logger.warning() instead of print() to stderr. Addresses Copilot review feedback on github#3966.
|
This is a logging-consistency change rather than a demonstrated functional fix, so we’re keeping it as The specific test mismatch from the earlier review remains: Please also state whether AI assistance was used and, if so, include the agent/tool, model, mode/settings, and extent of assistance. Drafted for @mnriem with assistance from GitHub Copilot (model: GPT-6 Astra; interactive comment drafting). |
mnriem
added
triage-can-wait
There was a problem hiding this comment.
🟡 Changes recommended
An existing stderr-based test will fail, and the docstring still incorrectly guarantees stderr output.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
src/specify_cli/_agent_config.py:46
- The function docstring still promises that invalid values emit a warning to stderr, but
logger.warning()routes output through configured logging handlers and does not guarantee stderr. Describe this as a logged warning so the public contract matches the implementation.
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
| with caplog.at_level(logging.WARNING): | ||
| with patch.dict(os.environ, {"SPECKIT_INTEGRATION_DEFAULT": "nonexistent"}): | ||
| result = resolve_default_init_integration() | ||
| assert result == "copilot" # default fallback | ||
| assert any("nonexistent" in record.message for record in caplog.records) |
|
Please address Copilot feedback and fix test & lint errors |
fix: update docstring to reflect logging module usage
Problem
_agent_config.py used print(..., file=sys.stderr) for warnings instead of using the logging module.
Fix
Add logging.getLogger(name) and replace print() with logger.warning(). Remove unused sys import.