Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
68213010:
Clearer help in gclient-new-workdir.py (Closed)
|
Created:
7 years, 1 month ago by pgervais Modified:
7 years, 1 month ago CC:
chromium-reviews, Dirk Pranke, cmp-cc_chromium.org, M-A Ruel, iannucci+depot_tools_chromium.org, ilevy-cc_chromium.org Base URL:
https://chromium.googlesource.com/chromium/tools/depot_tools.git@master Visibility:
Public. |
DescriptionClearer help in gclient-new-workdir.py
This modification make gclient-new-workdir.py display explicit error messages instead of cryptic Python backtraces.
BUG=
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=235408
Patch Set 1 #
Total comments: 4
Patch Set 2 : Modification after maruel review #
Total comments: 4
Patch Set 3 : Made help clearer, style updates #
Total comments: 5
Patch Set 4 : Clearer help messages #
Total comments: 2
Messages
Total messages: 17 (0 generated)
https://codereview.chromium.org/68213010/diff/1/gclient-new-workdir.py File gclient-new-workdir.py (right): https://codereview.chromium.org/68213010/diff/1/gclient-new-workdir.py#newcode11 gclient-new-workdir.py:11: import os.path Not needed. https://codereview.chromium.org/68213010/diff/1/gclient-new-workdir.py#newcode18 gclient-new-workdir.py:18: if sys.platform.startswith("win"): On windows, it'll always be 'win32', so use: if sys.platform == 'win32': https://codereview.chromium.org/68213010/diff/1/gclient-new-workdir.py#newcode19 gclient-new-workdir.py:19: print("This script cannot run on Windows because it uses symlinks.") you should print to sys.stderr. I see the rest of the code didn't do that properly. https://codereview.chromium.org/68213010/diff/1/gclient-new-workdir.py#newcode34 gclient-new-workdir.py:34: print("First argument (repository) must be an absolute path.") Why not argv[1] = os.path.abspath(argv[1]) ?
On 2013/11/12 17:40:06, M-A Ruel wrote: > you should print to sys.stderr. I see the rest of the code didn't do that > properly. > I fixed that. > Why not > argv[1] = os.path.abspath(argv[1]) > ? I didn't want to modify the behavior of the script, but this makes sense. Corrected. Patch 2 ready for review.
I don't expect Stefan to do reviews in general, so sending CL to Robbie, as I try to lower my reviews in depot_tools. Robbie is also the one who did the initial review as per git log. Robbie, double-quotes? No module docstring? :) https://codereview.chromium.org/68213010/diff/50001/gclient-new-workdir.py File gclient-new-workdir.py (right): https://codereview.chromium.org/68213010/diff/50001/gclient-new-workdir.py#ne... gclient-new-workdir.py:25: Clone an existing gclient directory, taking care of all sub-repositories You want the text to be +4 at output?
On 2013/11/13 17:17:32, M-A Ruel wrote: > I don't expect Stefan to do reviews in general, so sending CL to Robbie, as I > try to lower my reviews in depot_tools. Robbie is also the one who did the > initial review as per git log. > > Robbie, double-quotes? No module docstring? :) > My mental pylint module is not as strong as yours :p. Seriously though, we should fix the tools to catch that stuff. > https://codereview.chromium.org/68213010/diff/50001/gclient-new-workdir.py > File gclient-new-workdir.py (right): > > https://codereview.chromium.org/68213010/diff/50001/gclient-new-workdir.py#ne... > gclient-new-workdir.py:25: Clone an existing gclient directory, taking care of > all sub-repositories > You want the text to be +4 at output?
https://codereview.chromium.org/68213010/diff/50001/gclient-new-workdir.py File gclient-new-workdir.py (right): https://codereview.chromium.org/68213010/diff/50001/gclient-new-workdir.py#ne... gclient-new-workdir.py:19: "symlinks.") As M-A pointed out, could convert all "" quoted strings to '' quoted strings? (at least the ones you touched, if you don't want to change the whole file. Also, I prefer print >> ..., ( "This .... symlinks.") especially since the string could all fit on a line. https://codereview.chromium.org/68213010/diff/50001/gclient-new-workdir.py#ne... gclient-new-workdir.py:25: Clone an existing gclient directory, taking care of all sub-repositories On 2013/11/13 17:17:33, M-A Ruel wrote: > You want the text to be +4 at output? I think you want to pass this string through http://docs.python.org/2/library/textwrap.html#textwrap.dedent so that you don't get awkward indentation. https://codereview.chromium.org/68213010/diff/50001/gclient-new-workdir.py#ne... gclient-new-workdir.py:41: print >> sys.stderr, ("New workdir already exists: " + new_workdir) What do you think about a print_err function do this?
On 2013/11/13 17:52:35, iannucci wrote: > On 2013/11/13 17:17:32, M-A Ruel wrote: > > I don't expect Stefan to do reviews in general, so sending CL to Robbie, as I > > try to lower my reviews in depot_tools. Robbie is also the one who did the > > initial review as per git log. > > > > Robbie, double-quotes? No module docstring? :) > > > > My mental pylint module is not as strong as yours :p. > > Seriously though, we should fix the tools to catch that stuff. And this was a python script from a relative python newbie. Sorry. Also, I believe it was wrong to symlink the .gclient_entries file instead of copying it.
On 2013/11/13 17:59:23, atreat wrote: > On 2013/11/13 17:52:35, iannucci wrote: > > On 2013/11/13 17:17:32, M-A Ruel wrote: > > > I don't expect Stefan to do reviews in general, so sending CL to Robbie, as > I > > > try to lower my reviews in depot_tools. Robbie is also the one who did the > > > initial review as per git log. > > > > > > Robbie, double-quotes? No module docstring? :) > > > > > > > My mental pylint module is not as strong as yours :p. > > > > Seriously though, we should fix the tools to catch that stuff. > > And this was a python script from a relative python newbie. Sorry. No need to apologize, we're all newbies at some point :) > > Also, I believe it was wrong to symlink the .gclient_entries file instead of > copying it. Good catch. In fact, I would even be inclined to let gclient re-generate a new one.
I fixed the code following your remarks (also replaced one invocation of os.mkdir by os.makedirs) On 2013/11/13 18:15:45, iannucci wrote: > On 2013/11/13 17:59:23, atreat wrote: > > > > Also, I believe it was wrong to symlink the .gclient_entries file instead of > > copying it. > > Good catch. In fact, I would even be inclined to let gclient re-generate a new > one. Fixed that too. The script does not symlink .gclient_entries anymore. Please be aware that the initial intend was just to improve error messages to help newbies like me. I don't want to spend too much time on this issue, even if tidying up is always a great thing.
last round! https://codereview.chromium.org/68213010/diff/140001/gclient-new-workdir.py File gclient-new-workdir.py (right): https://codereview.chromium.org/68213010/diff/140001/gclient-new-workdir.py#n... gclient-new-workdir.py:38: sys.exit(1) move this whole if statement body into a usage() function which prints usage and quits w/ 1. The usage function should take an optional msg argument, and print that on stderr before printing usage. usage should probably be printed on stderr as well, right? https://codereview.chromium.org/68213010/diff/140001/gclient-new-workdir.py#n... gclient-new-workdir.py:45: sys.exit(1) then these can become usage(...) calls https://codereview.chromium.org/68213010/diff/140001/gclient-new-workdir.py#n... gclient-new-workdir.py:62: os.symlink(gclient, os.path.join(new_workdir, '.gclient')) I'm assuming you tried it out and this does, in fact, work (i.e. gclient generates a new entries file)
https://codereview.chromium.org/68213010/diff/140001/gclient-new-workdir.py File gclient-new-workdir.py (right): https://codereview.chromium.org/68213010/diff/140001/gclient-new-workdir.py#n... gclient-new-workdir.py:38: sys.exit(1) On 2013/11/13 20:29:10, iannucci wrote: > move this whole if statement body into a usage() function which prints usage and > quits w/ 1. The usage function should take an optional msg argument, and print > that on stderr before printing usage. usage should probably be printed on stderr > as well, right? It depends if execution of usage() is triggered by a -h switch or by invalid parameters. Since there is not -h option here, I suppose printing to stderr is fine. https://codereview.chromium.org/68213010/diff/140001/gclient-new-workdir.py#n... gclient-new-workdir.py:62: os.symlink(gclient, os.path.join(new_workdir, '.gclient')) On 2013/11/13 20:29:10, iannucci wrote: > I'm assuming you tried it out and this does, in fact, work (i.e. gclient > generates a new entries file) Yes it does (on gclient sync at least)
Last patch uploaded!
lgtm % comment (I could be convinced to leave it as-is if the UX is vastly superior, but it's a bit of a weird usage() function). thanks for working on this. https://codereview.chromium.org/68213010/diff/190001/gclient-new-workdir.py File gclient-new-workdir.py (right): https://codereview.chromium.org/68213010/diff/190001/gclient-new-workdir.py#n... gclient-new-workdir.py:25: usage_msg = 'Run without arguments to get usage help.' oh, hm... I would just remove this line and the else: Then if you screw up, you get the error message, followed by usage. I don't have the CLI in front of me though, so maybe that's a bad UX?
One last round of discussion, before committing, because I think your point is valid: https://codereview.chromium.org/68213010/diff/190001/gclient-new-workdir.py File gclient-new-workdir.py (right): https://codereview.chromium.org/68213010/diff/190001/gclient-new-workdir.py#n... gclient-new-workdir.py:25: usage_msg = 'Run without arguments to get usage help.' On 2013/11/14 01:45:30, iannucci wrote: > oh, hm... I would just remove this line and the else: > > Then if you screw up, you get the error message, followed by usage. I don't have > the CLI in front of me though, so maybe that's a bad UX? I did that because when you screw up, what you see most is the usage help, and the error message is hardly visible, at the very top. This was my $0.02 to get the error message more visible. Another possibility is to show the error message as the last line, but I didn't find it very convincing either.
On 2013/11/15 19:38:00, pgervais wrote: > One last round of discussion, before committing, because I think your point is > valid: > > https://codereview.chromium.org/68213010/diff/190001/gclient-new-workdir.py > File gclient-new-workdir.py (right): > > https://codereview.chromium.org/68213010/diff/190001/gclient-new-workdir.py#n... > gclient-new-workdir.py:25: usage_msg = 'Run without arguments to get usage > help.' > On 2013/11/14 01:45:30, iannucci wrote: > > oh, hm... I would just remove this line and the else: > > > > Then if you screw up, you get the error message, followed by usage. I don't > have > > the CLI in front of me though, so maybe that's a bad UX? > > I did that because when you screw up, what you see most is the usage help, and > the error message is hardly visible, at the very top. This was my $0.02 to get > the error message more visible. Another possibility is to show the error message > as the last line, but I didn't find it very convincing either. OK, that sgtm then :).
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pgervais@chromium.org/68213010/190001
Message was sent while issue was closed.
Change committed as 235408 |
