-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
unify cli verbosity handling #3296
Conversation
d7b9744
to
c1b14cf
Compare
Thank you very much for working on this!! |
f1a8174
to
1dccfa4
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.
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 |
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.
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.
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.
its random shitty data showing up in introspection
on top of that its half-dead zombie data and partially incorrect, it should go away
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.
We can't we remove it now? Is it because it might be used by thirdparty plugins?
import argparse | ||
|
||
|
||
class MoreQuietAction(argparse.Action): |
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.
Please add a docstring documenting why this class exists.
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.
I don't understand why we need this class. What does it fix?
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.
it implements a negative counter and backward compat hacking, it can be simplified later on but its necessary since argparse itself is kinda shitty ^^
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.
ah. It is needed to get rid of config.option.verbose -= config.option.quiet
, right?
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.
yup
_pytest/3296.trivial
Outdated
@@ -0,0 +1 @@ | |||
unify cli verbosity handling |
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.
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.
1dccfa4
to
16779f3
Compare
based on pytest-dev#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
16779f3
to
37a5260
Compare
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