Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Verbosity now affects package installation #403

Merged
merged 1 commit into from
Nov 7, 2016
Merged

Verbosity now affects package installation #403

merged 1 commit into from
Nov 7, 2016

Conversation

rogalski
Copy link

@rogalski rogalski commented Nov 5, 2016

Solves #353.

hpk42
hpk42 reviewed Nov 6, 2016
View reviewed changes
tox/session.py
@property
def verbose(self):
return self._get_verbosity() >= 2

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this property doesn't make much sense to me. Why not remake _get_verbosity into a verbosity property and use a proper <2 comparison further below? This makes it more obvious when redirection takes place and when not.

@hpk42
Copy link

hpk42 commented Nov 6, 2016

Thanks for the PR. See one inline comment. I wonder if some people will be surprised that -vv turns of redirection. If so we might have to rather go for a separate option. Please amend the help string of -v to say that if it's supplied twice output from commands will be shown, not redirected to a file. This way people can discover it via tox -h | grep output or so.

@The-Compiler
Copy link
Member

FWIW I'm surprised by the current behaviour - I'd absolutely expect -vv to show me pip install output 😉

@rogalski
Copy link
Author

rogalski commented Nov 6, 2016

After code review is completed, I'll happily squash and rebase change, since right now history is quite messy.

@hpk42
Copy link

hpk42 commented Nov 6, 2016

Looks good, thanks! Please rebase and then we can merge.

@rogalski
Package installation output is now affected by verbosity (-vv)
a4bc5fc 
Solves issue tox-dev#353.

Add changelog and contributors entry

Add integration tests

expose _get_verbosity() in public api, improve docs and help messages
@rogalski rogalski force-pushed the verbosity_affects_installation branch from 8704d74 to a4bc5fc Compare November 6, 2016 20:39
@nicoddemus nicoddemus merged commit 60d0cd2 into tox-dev:master Nov 7, 2016
@nicoddemus
Copy link
Member

Went ahead with @hpk42's instructions. 👍

@The-Compiler The-Compiler mentioned this pull request Nov 7, 2016
Closed
@rogalski rogalski deleted the verbosity_affects_installation branch November 13, 2016 11:43
@MartinHjelmare MartinHjelmare mentioned this pull request Nov 19, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hpk42 hpk42 hpk42 left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@rogalski @hpk42 @The-Compiler @nicoddemus