Better message when missing a required command line tool. #455
Conversation
| wide_log(clr("[build] - `{}`".format(bt_name))) | ||
|
|
||
| except CommandMissing as e: | ||
| sys.exit(clr("[build] @!@{rf}Error:@| {0}").format(e)) |
There was a problem hiding this comment.
Intent here would be to use a tuple ultimately to catch other kinds of exceptions that may be necessary to throw for problems discovered during job creation.
There was a problem hiding this comment.
This looks ok, but what do you think about putting this try catch around all of the the build_isolated_workspace function? Indenting this much code is just making it harder to read imo, and I think it should be ok to include the rest of the function in this try-catch right?
There was a problem hiding this comment.
You mean moving it to catkin_build/cli.py? That can be done. It'll mean importing the exception there instead of here, which probably doesn't really matter.
The main thing is not catching everything— we still want stack traces for unknown problems.
There was a problem hiding this comment.
Yeah, that also sounds fine, though I was talking about just making a new function here that calls the current function in a try-catch, but your suggestion makes more sense.
In the end it's a small thing, but among other things it makes this diff hard to read.
There was a problem hiding this comment.
It would be great if git/github's diff were smarter about recognizing blocks that have changed only in indentation.
There was a problem hiding this comment.
Not sure whether there is any UI for this, but you can get the diff renderer to ignore whitespace changes by appending w=1 to the url: https://github.com/catkin/catkin_tools/pull/455/files?w=1.
|
|
||
| def __init__(self, name): | ||
| super(CommandMissing, self).__init__( | ||
| 'Cannot find required tool `%s` on the PATH, is it installed?' % name) |
There was a problem hiding this comment.
Happy to move this to its own exceptions.py file, didn't want to bother with that unless there was a clear need.
There was a problem hiding this comment.
(A good motivation to do so might be eliminating the use of sys.exit within build_isolated_workspace, in favour of throwing other exceptions that resolve to the string error messages. Doing this would make the function more cooperative as far as being wrapped or called from other tools.)
mikepurvis
force-pushed
the
better-missing-command-msg
branch
from
240bf73 to
7d687ab
Compare
catkin_tools/jobs/utils.py
Outdated
|
|
||
| def require_command(name, which): | ||
| if not which: | ||
| import errno |
There was a problem hiding this comment.
Oh fail, that's a leftover— an earlier version of this threw OSError instead. Removed.
There was a problem hiding this comment.
The function is simple enough now that it could probably just be inlined; I don't really care one way or another about that.
| wide_log(clr("[build] - `{}`".format(bt_name))) | ||
|
|
||
| except CommandMissing as e: | ||
| sys.exit(clr("[build] @!@{rf}Error:@| {0}").format(e)) |
There was a problem hiding this comment.
This looks ok, but what do you think about putting this try catch around all of the the build_isolated_workspace function? Indenting this much code is just making it harder to read imo, and I think it should be ok to include the rest of the function in this try-catch right?
mikepurvis
force-pushed
the
better-missing-command-msg
branch
from
7d687ab to
4286b08
Compare
mikepurvis
force-pushed
the
better-missing-command-msg
branch
from
4286b08 to
846ee01
Compare
|
Okay, I think this is good now. |
Looks like this:
Fixes #437.