-
Notifications
You must be signed in to change notification settings - Fork 2.4k
outputs 'env activate' verbose message on stderr #10353
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
outputs 'env activate' verbose message on stderr #10353
Conversation
Reviewer's Guide by SourceryThis pull request changes the output stream of the virtual environment path message from stdout to stderr when running in verbose mode. This change was accompanied by a test update to ensure the correct output stream is being used. No diagrams generated as the changes look simple and do not need a visual representation. File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
Hey @deronnax - I've reviewed your changes - here's some feedback:
Overall Comments:
- It's great that you're directing the verbose message to stderr, as it's the correct stream for informational messages that don't represent standard output.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
81bde29
to
403f949
Compare
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.
Can you add a test in test_activate.py that checks that the output only contains the desired line and not any additional information in verbose mode?
I added the verbose mode in the existing tests, it seems better to me than adding an extra test just for this. |
Agreed.
Strictly speaking, we cannot be sure that it still works without verbose output if we just test the verbose mode. Thus, I think it makes sense to test both (parametrized). However, even more important, we have to use an |
d3eb200
to
8fd973c
Compare
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Pull Request Check List
Resolves: #10351
I don't think it needs a documentation change (this corner case wasn't documented in the first place).
Summary by Sourcery
Modify Poetry's run command to output the virtualenv activation message to stderr instead of stdout when in verbose mode
Bug Fixes:
Tests: