-
-
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
Fix exit code for command line errors #3925
Conversation
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.
Thaks a lot @crazymerlyn!
I'm surprised we don't have any tests for this. Please add a test which calls pytest with the wrong arguments and checks the correct return code. I believe something like this in acceptance_test.py
would suffice:
def test_usage_error_code(testdir):
result = testdir.runpytest('-unknown-option-')
assert result.ret == EXIT_USAGEERROR
Oh and I forgot: please target the |
6a19999
to
b01704c
Compare
Codecov Report
@@ Coverage Diff @@
## features #3925 +/- ##
============================================
+ Coverage 96.24% 96.24% +<.01%
============================================
Files 108 108
Lines 23409 23419 +10
============================================
+ Hits 22529 22539 +10
Misses 880 880
Continue to review full report at Codecov.
|
Changelog entry fixed, test added and the pull request now targets the features branch. |
Thanks @crazymerlyn! Closing/reopening the PR to restart CI. |
@@ -2,8 +2,13 @@ | |||
import warnings | |||
import argparse | |||
|
|||
from gettext import gettext as _ |
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.
pytest does not use gettext at all so why add it
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.
Oh right, totally missed that. Weird though, shouldn't flake8 catch that?
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.
@RonnyPfannschmidt The default implementation of error function used it and I didn't want to change anything other than the error code.
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.
Oh right, it is used:
self.exit(EXIT_USAGEERROR, _("%(prog)s: error: %(message)s\n") % args)
Even though we don't translate pytest messages, I don't mind leaving it in as gettext
is already being imported by argparser
anyway. But want me to remove it @RonnyPfannschmidt?
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.
@crazymerlyn thanks for the additional context
@nicoddemus thanks for the investigation
im a bit thorn between keeping similarity to imported code and limiting code scope, lets keep it as is for now
Fixes #3913