-
Notifications
You must be signed in to change notification settings - Fork 3.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
CLI: log to stderr instead of stdout #8784
Conversation
WalkthroughThe changes in this pull request focus on modifying the logging behavior of the command-line interface (CLI) and enhancing the output of task creation feedback. Specifically, log messages are now directed to standard error (stderr) instead of standard output (stdout). Additionally, the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant TaskImport
User->>CLI: Execute task import
CLI->>TaskImport: Call execute method
TaskImport->>TaskImport: Create task from backup
TaskImport-->>CLI: Return created task
CLI->>User: Print created task ID
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Since the `ls` command produces machine-readable output, we need to ensure that any log messages produced do not corrupt that output. Logging to stderr is also conventional; Python's `StreamHandler` even defaults to it. This breaks the `import` command tests, because previously they were looking at the last log message. I don't think we should be testing log messages, so add a proper `print` to this command. This also makes it consistent with the `create` command.
Quality Gate passedIssues Measures |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
cvat-cli/src/cvat_cli/_internal/commands.py (1)
421-426
: LGTM! Minor formatting suggestion.The changes improve output consistency with other commands and properly capture the task creation result. Consider removing the redundant string "ID" to match the exact format used in TaskCreate.
- print(f"Created task ID", task.id) + print("Created task id", task.id)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
changelog.d/20241206_135902_roman_cli_logging.md
(1 hunks)cvat-cli/src/cvat_cli/_internal/commands.py
(1 hunks)cvat-cli/src/cvat_cli/_internal/common.py
(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- cvat-cli/src/cvat_cli/_internal/common.py
- changelog.d/20241206_135902_roman_cli_logging.md
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #8784 +/- ##
===========================================
+ Coverage 73.92% 73.93% +0.01%
===========================================
Files 409 409
Lines 43944 43945 +1
Branches 3985 3985
===========================================
+ Hits 32485 32492 +7
+ Misses 11459 11453 -6
|
Motivation and context
Since the
ls
command produces machine-readable output, we need to ensure that any log messages produced do not corrupt that output.Logging to stderr is also conventional; Python's
StreamHandler
even defaults to it.This breaks the
import
command tests, because previously they were looking at the last log message. I don't think we should be testing log messages, so add a properprint
to this command. This also makes it consistent with thecreate
command.How has this been tested?
Unit tests.
Checklist
develop
branch[ ] I have updated the documentation accordingly[ ] I have added tests to cover my changes[ ] I have linked related issues (see GitHub docs)[ ] I have increased versions of npm packages if it is necessary(cvat-canvas,
cvat-core,
cvat-data and
cvat-ui)
License
Feel free to contact the maintainers if that's a concern.
Summary by CodeRabbit
New Features
TaskImport
command by displaying the created task ID after task creation.Bug Fixes
Documentation