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

unify cli verbosity handling #3296

Merged
merged 1 commit into from Mar 13, 2018

Conversation

Projects
None yet
4 participants
@RonnyPfannschmidt
Member

RonnyPfannschmidt commented Mar 12, 2018

based on #3294 (comment)

we really shouldnt have N options we post mortem hack together to determine verbosity
this change starts by unifying the data, we still need to handle deprecation/removal of config.quiet

@thisch

This comment has been minimized.

Contributor

thisch commented Mar 12, 2018

Thank you very much for working on this!!

@coveralls

This comment has been minimized.

coveralls commented Mar 12, 2018

Coverage Status

Coverage increased (+0.06%) to 92.971% when pulling 37a5260 on RonnyPfannschmidt:verbosity-actions into f8791c9 on pytest-dev:features.

@RonnyPfannschmidt RonnyPfannschmidt requested a review from nicoddemus Mar 12, 2018

@nicoddemus

Aside from my minor comments, we also need to add tests.

new_count = getattr(namespace, self.dest, 0) - 1
setattr(namespace, self.dest, new_count)
# todo Deprecate config.quiet
namespace.quiet = getattr(namespace, 'quiet', 0) + 1

This comment has been minimized.

@nicoddemus

nicoddemus Mar 12, 2018

Member

Hmm do we really need to deprecate namespace.quiet? If all we need to do is to keep this line around, I wouldn't bother TBH.

This comment has been minimized.

@RonnyPfannschmidt

RonnyPfannschmidt Mar 13, 2018

Member

its random shitty data showing up in introspection

on top of that its half-dead zombie data and partially incorrect, it should go away

This comment has been minimized.

@thisch

thisch Mar 13, 2018

Contributor

We can't we remove it now? Is it because it might be used by thirdparty plugins?

import argparse
class MoreQuietAction(argparse.Action):

This comment has been minimized.

@nicoddemus

nicoddemus Mar 12, 2018

Member

Please add a docstring documenting why this class exists.

This comment has been minimized.

@thisch

thisch Mar 13, 2018

Contributor

I don't understand why we need this class. What does it fix?

This comment has been minimized.

@RonnyPfannschmidt

RonnyPfannschmidt Mar 13, 2018

Member

it implements a negative counter and backward compat hacking, it can be simplified later on but its necessary since argparse itself is kinda shitty ^^

This comment has been minimized.

@thisch

thisch Mar 13, 2018

Contributor

ah. It is needed to get rid of config.option.verbose -= config.option.quiet, right?

This comment has been minimized.

@RonnyPfannschmidt
@@ -0,0 +1 @@
unify cli verbosity handling

This comment has been minimized.

@nicoddemus

nicoddemus Mar 12, 2018

Member

I would write something that makes sense to users:

Refactoring to unify how verbosity is handled internally.

And we also need a new feature entry:

New ``--verbosity`` flag to set verbosity level explicitly.
unify cli verbosity handling
based on #3294 (comment)

we really shouldnt have N options we post mortem hack together to determine verbosity
this change starts by unifying the data, we still need to handle deprecation/removal of config.quiet

@nicoddemus nicoddemus merged commit 4175ed8 into pytest-dev:features Mar 13, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@RonnyPfannschmidt RonnyPfannschmidt deleted the RonnyPfannschmidt:verbosity-actions branch Oct 1, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment