-
-
Notifications
You must be signed in to change notification settings - Fork 76
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
Implement setup.py runner #165
Conversation
2 similar comments
Just a quick note: I have been traveling yesterday and today, and tomorrow is a holiday (in the United States), so I will probably find some time to review (and hopefully merge) this in a couple days. Sorry for the delay! |
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.
Nice work!
I only found minor things to adjust -- I added notes to the PR for those things.
Two major things to ADD:
- Documentation somewhere in the
--help
output that documents this new feature. Probably in theepilog
(config.py:99
) - A subsection in the Integration section of
README.md
also documenting the feature. I'd put it after the### Coverage
section somewhere.
|
||
options = [] | ||
|
||
for action in r.store_opt.actions: |
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.
Crash here with distutils on Python 2.7.
This fixes it:
diff --git a/green/command.py b/green/command.py
index e628d16..1b19922 100644
--- a/green/command.py
+++ b/green/command.py
@@ -18,9 +18,9 @@ def get_user_options():
options = []
for action in r.store_opt.actions:
- names = [name.lstrip('-') for name in action.option_strings]
+ names = [str(name.lstrip('-')) for name in action.option_strings]
if len(names) == 1: names.insert(0, None)
- if not action.const: names[1] += "="
+ if not action.const: names[1] += str("=")
options.append((names[1], names[0], action.help))
return options
green/cmdline.py
Outdated
@@ -13,8 +13,8 @@ | |||
import green.config as config | |||
|
|||
|
|||
def main(testing=False): | |||
args = config.parseArguments() | |||
def main(argv=sys.argv[1:], testing=False): |
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.
Tiny thing: argv
here should default to None
since the intent is to provide an optional override to be used with the new feature. None
ensures we keep the same old default behavior even if argparse
decides to change what parse_args(None)
means in the distant future.
green/cmdline.py
Outdated
@@ -23,7 +23,7 @@ def main(testing=False): | |||
# Clear out all the passed-in-options just in case someone tries to run a | |||
# test that assumes sys.argv is clean. I can't guess at the script name | |||
# that they want, though, so we'll just leave ours. | |||
sys.argv = sys.argv[:1] | |||
#sys.argv = sys.argv[: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.
Changing the clean-sys-argv behavior is outside the scope of distcmd feature. Go ahead and uncomment-out this line, your feature works with the line in place (I tested it to make sure).
If you want to change the clean-sys-arg feature specifically for some reason, I'd be happy to discuss that separately from this PR.
self.ensure_finalized() | ||
|
||
if self.distribution.install_requires: | ||
self.distribution.fetch_build_eggs( |
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.
Missing coverage on this line.
self.distribution.fetch_build_eggs( | ||
self.distribution.install_requires) | ||
if self.distribution.tests_require: | ||
self.distribution.fetch_build_eggs( |
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.
Missing coverage on this line as well.
green/config.py
Outdated
@@ -293,7 +294,7 @@ def parseArguments(): # pragma: no cover | |||
help="Output all options. Used by bash- and zsh-completion.", | |||
default=argparse.SUPPRESS)) | |||
|
|||
args = parser.parse_args() | |||
args = parser.parse_args(argv or None) |
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.
Since argv
is already a list or None
, you can simplify this slightly to:
args = parser.parse_args(argv)
green/test/test_djangorunner.py
Outdated
@@ -137,7 +137,7 @@ def test_check_verbosity_argument_recognised(self): | |||
test_command.test_runner = "green.djangorunner.DjangoRunner" | |||
parser = ArgumentParser() | |||
test_command.add_arguments(parser) | |||
args = parser.parse_args() | |||
args = parser.parse_args([]) |
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.
This change is not needed. Lets drop the change.
green/test/test_djangorunner.py
Outdated
@@ -154,7 +154,7 @@ def test_check_default_verbosity(self): | |||
test_command.test_runner = "green.djangorunner.DjangoRunner" | |||
parser = ArgumentParser() | |||
test_command.add_arguments(parser) | |||
args = parser.parse_args() | |||
args = parser.parse_args([]) |
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.
This change is not needed. Lets drop the change.
I'll add the doc, although I have the feeling it is quite dense already. I'd like to suggest a man page or an help with two levels (commands only / features); this goes out of the scope of the PR but I just wanted you to know that 😉 |
2 similar comments
Build fails because of Travis, not me, seems like there is an infrastructure error. |
I agree with you. Travis is the problem in this case. Once again, great work! |
Included in version 2.10.0, just released. |
Great ! Thanks ;) |
PR for #158.
Allows running green with
python setup.py green
by declaring a distutils command ingreen.command
.The list of arguments is the same as the arguments of the plain green CLI, as it is dynamically generated using the
StoreOpt
results.I had to change the behaviour of
green.main
a bit, to make it possible to pass it a customargv
. The command only validates the input arguments, and then passes everything togreen.main
.Since distutils commands cannot have positional arguments, I made it possible to specify a test target to use using the
test_suite
argument in thesetup.py
.