-
-
Notifications
You must be signed in to change notification settings - Fork 289
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
Molsen/add pex root option #206
Changes from 4 commits
722d225
6111558
71be040
8fa6014
35bd238
64aedc9
d10a7e3
f079a2c
2b58901
6538ff6
f4e4ef1
e56f439
556a30a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -503,11 +503,18 @@ def make_relative_to_root(path): | |
return os.path.normpath(path.format(pex_root=ENV.PEX_ROOT)) | ||
|
||
|
||
def main(): | ||
def main(args=None, log_callback=None): | ||
if log_callback is not None: | ||
log = log_callback | ||
|
||
parser, resolver_options_builder = configure_clp() | ||
|
||
# split arguments early because optparse is dumb | ||
args = sys.argv[1:] | ||
if args is None: | ||
# split arguments early because optparse is dumb | ||
args = sys.argv[1:] | ||
else: | ||
args = args[:] | ||
|
||
try: | ||
separator = args.index('--') | ||
args, cmdline = args[:separator], args[separator + 1:] | ||
|
@@ -542,7 +549,7 @@ def main(): | |
|
||
log('Running PEX file at %s with args %s' % (pex_builder.path(), cmdline), v=options.verbosity) | ||
pex = PEX(pex_builder.path(), interpreter=pex_builder.interpreter) | ||
sys.exit(pex.run(args=list(cmdline))) | ||
raise SystemExit(pex.run(args=list(cmdline))) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this change still feels weird to me - and I think to others it will also. since the two are fundamentally identical, why not just stick with the more intuitive/common form of fwiw, you shouldn't need to explicitly raise here to be able to handle SystemExit in your tests:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No strong objection. I'll update the code to use sys.exit rather than raise SystemExit. |
||
|
||
|
||
if __name__ == '__main__': | ||
|
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.
not crazy about this bit, how about just:
and then eliminating all of the
if log_callback
&if args
checks + theraise SystemExit
in favor of just passing in test-context values for these?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'd rather not put mutable objects in as the default options, which is why I went with None. As far as the sys.argv[1:] being placed in main I'd rather not put that logic somewhere it can't be easily tested.
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'm not sure I understand your statements or reasoning here.
"I'd rather not put mutable objects in as the default options, which is why I went with None." -> what default option here is mutable?
log
is a function defined above.sys.exit
is also a function. if anything, the whole 'log can be either be a global or local depending on the value of log_callback' strikes me as most peculiar."As far as the sys.argv[1:] being placed in main I'd rather not put that logic somewhere it can't be easily tested." -> somehow I feel like the slicing of
sys.argv
isn't a super critical thing to test? also, by precedent injection of args is actually a super common pattern intwitter.common.app
. what's the theoretical harm here in your mind? or why not alternativelymain(sys.argv)
?I'm a firm -1 on avoiding needless clutter in
main()
for tests and particularly not a fan of this approach as-is - would prefer that you either clean this up in-line with the suggestions or find an alternate route.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.
mutable object was in ref to sys.argv. I generally think that the slice should still be in testable code it would make a difference for example if you were to change option frameworks to docopt or argparse. However, that being said its not super critical and I've moved it.
Also removed the log=None default.
Spoke with you offline and you generally seem okay with SystemExit approach.
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.
looking through this, updating log in main() doesn't actually help since several other methods are using log as a global as well. I'll update the code later with an alternative that allows you to update the logger for all methods.