Issue 29371804: Noissue - [hgreview] Only use first line of commit message as review title (Closed)
|
Created:
Jan. 13, 2017, 1:57 p.m. by f.nicolaisen Modified:
July 19, 2017, 8:33 p.m. Base URL:
https://hg.adblockplus.org/codingtools/ Visibility:
Public. |
DescriptionNoissue - [hgreview] Only use first line of commit message as review title
As you can see in this review (which was uploaded using the patched review.py),
only the first line of the message is present in the review title, whereas the
complete commit message is present in description. Very meta.
Patch Set 1 #
Total comments: 2
Patch Set 2 : follow Vasily's suggestions #
Total comments: 5
Patch Set 3 : Trying Wladimir's idea supporting multiline from command line #
Total comments: 6
Patch Set 4 : Addressing latest review from Wladimir #
Total comments: 4
Patch Set 5 : Trying again #
Total comments: 4
Patch Set 6 : Remove unnecessary condition #MessagesTotal messages: 23
Hi Ferris, I think I found a problem with your change, see below. After that is out of the way, it would be good to add Wladimir to the review as he is the original author of hgreview and since here we're changing behavior, I'd like to have his feedback too. Cheers, Vasily https://codereview.adblockplus.org/29371804/diff/29371805/hgreview.py File hgreview.py (right): https://codereview.adblockplus.org/29371804/diff/29371805/hgreview.py#newcode62 hgreview.py:62: opts['title'] = repo[opts['change']].description().rstrip().split('\n')[0] There's a bit too much code repetition here and, more dangerously, we're overwriting opts['message'] even though the user might have supplied it. It seems better to do something like this: if opts.get('change'): description = repo[opts['change']].description() else: # Code from line 65, maybe the prompt can be changed... description = ui.prompt('New review title: ', '') # Here we'd have the check from line 66 applied to description instead. Then we can use `description` for providing default values for opts['title'] in line 62 and opts['message'] in line 69.
Thanks for reviewing! I've tried to address the points with PS 2. https://codereview.adblockplus.org/29371804/diff/29371805/hgreview.py File hgreview.py (right): https://codereview.adblockplus.org/29371804/diff/29371805/hgreview.py#newcode62 hgreview.py:62: opts['title'] = repo[opts['change']].description().rstrip().split('\n')[0] On 2017/01/13 17:58:08, Vasily Kuznetsov wrote: > There's a bit too much code repetition here and, more dangerously, we're > overwriting opts['message'] even though the user might have supplied it. > > It seems better to do something like this: > > if opts.get('change'): > description = repo[opts['change']].description() > else: > # Code from line 65, maybe the prompt can be changed... > description = ui.prompt('New review title: ', '') > > # Here we'd have the check from line 66 applied to description instead. > > Then we can use `description` for providing default values for opts['title'] in > line 62 and opts['message'] in line 69. Acknowledged.
https://codereview.adblockplus.org/29371804/diff/29371815/hgreview.py File hgreview.py (right): https://codereview.adblockplus.org/29371804/diff/29371815/hgreview.py#newcode64 hgreview.py:64: description = ui.prompt('New review title: ', '') The logic change here will have the effect that the prompt will appear even though title is set explicitly on the command line. Please revert that unrelated change, the original logic was correct.
https://codereview.adblockplus.org/29371804/diff/29371815/hgreview.py File hgreview.py (right): https://codereview.adblockplus.org/29371804/diff/29371815/hgreview.py#newcode64 hgreview.py:64: description = ui.prompt('New review title: ', '') On 2017/01/16 10:11:43, Wladimir Palant wrote: > The logic change here will have the effect that the prompt will appear even > though title is set explicitly on the command line. Please revert that unrelated > change, the original logic was correct. Yes, that's right. I also missed this case when suggesting the change. After playing around with different clever structures for this logic, I have this proposal: how about we revert to the original code and add one line before line 74 (it was line 71 in original code: path = ....): opts['title'] = opts['title'].splitlines()[0] # Title should be single line Seems like it would do the trick.
https://codereview.adblockplus.org/29371804/diff/29371815/hgreview.py File hgreview.py (right): https://codereview.adblockplus.org/29371804/diff/29371815/hgreview.py#newcode64 hgreview.py:64: description = ui.prompt('New review title: ', '') On 2017/01/16 10:46:07, Vasily Kuznetsov wrote: > proposal: how about we revert to the original code and add one line before line > 74 (it was line 71 in original code: path = ....): > > opts['title'] = opts['title'].splitlines()[0] # Title should be single line > > Seems like it would do the trick. This isn't the right thing to do either. We should not cut off a title that was passed in on the command line. Only the commit description we extract from repository needs to be cut off - so the original commit did the right thing for most part. Maybe something like this: fulltitle = None ... if not opts.get('title') and opts.get('change'): fulltitle = repo[opts['change']].description() opts['title'] = fulltitle.rstrip().split('\n')[0] ... if not opts.get('message'): opts['message'] = opts['title'] if fulltitle is None else fulltitle
https://codereview.adblockplus.org/29371804/diff/29371815/hgreview.py File hgreview.py (right): https://codereview.adblockplus.org/29371804/diff/29371815/hgreview.py#newcode64 hgreview.py:64: description = ui.prompt('New review title: ', '') On 2017/01/16 21:50:50, Wladimir Palant wrote: > On 2017/01/16 10:46:07, Vasily Kuznetsov wrote: > > proposal: how about we revert to the original code and add one line before > line > > 74 (it was line 71 in original code: path = ....): > > > > opts['title'] = opts['title'].splitlines()[0] # Title should be single > line > > > > Seems like it would do the trick. > > This isn't the right thing to do either. We should not cut off a title that was > passed in on the command line. Only the commit description we extract from > repository needs to be cut off - so the original commit did the right thing for > most part. Maybe something like this: > > fulltitle = None > ... > if not opts.get('title') and opts.get('change'): > fulltitle = repo[opts['change']].description() > opts['title'] = fulltitle.rstrip().split('\n')[0] > ... > if not opts.get('message'): > opts['message'] = opts['title'] if fulltitle is None else fulltitle My thinking was that it's quite unlikely that someone will pass a multiline title on the command line or would even want to have a multiline title at all. I also tried to not increase the complexity of this fragment (especially the second proposal I think did a good job at that). On the other hand you wrote this extension in the first place and you used it more than me so I'm inclined to weigh your opinion more than mine. If you think the use case with multiline title is important, I won't disagree.
https://codereview.adblockplus.org/29371804/diff/29371815/hgreview.py File hgreview.py (right): https://codereview.adblockplus.org/29371804/diff/29371815/hgreview.py#newcode64 hgreview.py:64: description = ui.prompt('New review title: ', '') On 2017/01/16 22:39:05, Vasily Kuznetsov wrote: > My thinking was that it's quite unlikely that someone will pass a multiline > title on the command line or would even want to have a multiline title at all. It's not impossible however - typically the data comes from a pipe then (e.g. via xargs).
One thing that seems to be missing here (to me) is the repository name. I often put the repository name in the title of my codereviews so that my reviewer knows which repository to clone and patch. But I don't want to put the repository name in my commit message.
On 2017/04/28 18:36:28, juliandoucette wrote: > One thing that seems to be missing here (to me) is the repository name. I often > put the repository name in the title of my codereviews so that my reviewer knows > which repository to clone and patch. But I don't want to put the repository name > in my commit message. Yes, repository name and base revision are quite useful. Here the repository name is actually displayed on the left (see "Base URL") -- hgreview.py puts it there if it can find it in .hg/hgrc. It would be nice to also have the base revision somewhere. But that would be a separate ticket I suppose.
I'm removing myself from reviewers. PS: Don't forget about this change guys.
On 2017/07/14 12:15:38, juliandoucette wrote: > I'm removing myself from reviewers. > > PS: Don't forget about this change guys. I tried implementing Wladimir's suggestion in PS3. I tested: hg review -c tip (typed ferris when prompted for reviewers) Here's what came out before uploading: Args: ['--oauth2', '--server', 'https://codereview.adblockplus.org', '--send_mail', '--rev', '8d5abf773533:c77716ff820c', '--title', 'Noissue - [hgreview] Only use first line of description as review title', '--message', ' Noissue - [hgreview] Only use first line of description as review title\n\nAs you can see in this review (which was uploaded using the patched review.py),\nonly the first line of the message is present in the review title, whereas the\ncom plete commit message is present in description. Very meta.', '--reviewers', 'ferris@adblockplus.org', '--base_url', 'https://hg.adblockplus.org/codingtools/'] Opts: {'print_diffs': None, 'cc': '', 'reviewers': 'ferris@adblockplus.or g', 'no_mail': None, 'title': 'Noissue - [hgreview] Only use first line of description as review title', 'assume_yes': None, 'private': None, 'base_url': 'https://hg.adblockplus.org/codingtools/', 'message': 'Noissue - [hgreview] Only use first line of description as review title\n\nAs you can see in this review (which was uploaded using the patched review.py),\nonly the first line of the message is present in the review title, whereas the\ncomplete commit message is presen t in description. Very meta.', 'issue': '', 'change': 'tip', 'revision': ''}. Then I tested with multiline: hg review -c tip -t "first line dquote> second line" Args: ['--oauth2', '--server', 'https://codereview.adblockplus.org', '--send_mail', '--rev', '8d5abf773533:37dcfcf0f0b9', '--title', 'first line\nsecond line', '--message', 'first line\nsecond line', '--reviewers', 'ferris@adblockplus.org', '--base_url', 'https://hg.adblockplus.org/codingtools/'] opts: {'print_diffs': None, 'cc': '', 'reviewers': 'ferris@adblockplus.org', 'no_mail': None, 'title': 'first line\nsecond line', 'assume_yes': None, 'private': None, 'base_url': 'https://hg.adblockplus.org/codingtools/', 'message': 'first line\nsecond line', 'issue': '', 'change': 'tip', 'revision': ''}. So, seems to be doing the right thing at least?
Maybe have a look at your entire changeset again? It has some leftover changes, see below. https://codereview.adblockplus.org/29371804/diff/29489571/hgreview.py File hgreview.py (right): https://codereview.adblockplus.org/29371804/diff/29489571/hgreview.py#newcode66 hgreview.py:66: description = ui.prompt('New review title: ', '') The variable description is being assigned but never used? https://codereview.adblockplus.org/29371804/diff/29489571/hgreview.py#newcode69 hgreview.py:69: opts['title'] = fulltitle.rstrip().split('\n')[0] So we are no longer asking for a title if neither --title nor --change are present? https://codereview.adblockplus.org/29371804/diff/29489571/hgreview.py#newcode74 hgreview.py:74: opts['message'] = opts['title'] if fulltitle is None else fulltitle How about: opts['message'] = fulltitle or opts['title']
Trying again with PS4 :) https://codereview.adblockplus.org/29371804/diff/29489571/hgreview.py File hgreview.py (right): https://codereview.adblockplus.org/29371804/diff/29489571/hgreview.py#newcode66 hgreview.py:66: description = ui.prompt('New review title: ', '') On 2017/07/17 13:20:37, Wladimir Palant wrote: > The variable description is being assigned but never used? Acknowledged. https://codereview.adblockplus.org/29371804/diff/29489571/hgreview.py#newcode69 hgreview.py:69: opts['title'] = fulltitle.rstrip().split('\n')[0] On 2017/07/17 13:20:37, Wladimir Palant wrote: > So we are no longer asking for a title if neither --title nor --change are > present? Acknowledged. https://codereview.adblockplus.org/29371804/diff/29489571/hgreview.py#newcode74 hgreview.py:74: opts['message'] = opts['title'] if fulltitle is None else fulltitle On 2017/07/17 13:20:37, Wladimir Palant wrote: > How about: > > opts['message'] = fulltitle or opts['title'] Acknowledged.
On 2017/07/17 13:43:41, f.nicolaisen wrote: > Trying again with PS4 :) Nope, try again please. Still too many changes. https://codereview.adblockplus.org/29371804/diff/29490692/hgreview.py File hgreview.py (right): https://codereview.adblockplus.org/29371804/diff/29490692/hgreview.py#newcode66 hgreview.py:66: fulltitle = ui.prompt('New review title: ', '') So we will be prompting even if --title is given on the command line? https://codereview.adblockplus.org/29371804/diff/29490692/hgreview.py#newcode68 hgreview.py:68: fulltitle = repo[opts['change']].description() Overwriting fulltitle variable with the same value as before?
Finally had to wip out the old boolean table. Getting rusty :) 1) If no change or title provider, prompt. 2) Else if change but no title, use (first line of) change. Keep fulltitle for message. Otherwise title is provided, so just use title. https://codereview.adblockplus.org/29371804/diff/29490692/hgreview.py File hgreview.py (right): https://codereview.adblockplus.org/29371804/diff/29490692/hgreview.py#newcode66 hgreview.py:66: fulltitle = ui.prompt('New review title: ', '') On 2017/07/17 14:57:52, Wladimir Palant wrote: > So we will be prompting even if --title is given on the command line? Acknowledged. https://codereview.adblockplus.org/29371804/diff/29490692/hgreview.py#newcode68 hgreview.py:68: fulltitle = repo[opts['change']].description() On 2017/07/17 14:57:52, Wladimir Palant wrote: > Overwriting fulltitle variable with the same value as before? Acknowledged.
Previous message is for PS5 btw.
Finally getting to the original logic, merely with the if conditions formulated differently :) https://codereview.adblockplus.org/29371804/diff/29491604/hgreview.py File hgreview.py (right): https://codereview.adblockplus.org/29371804/diff/29491604/hgreview.py#newcode65 hgreview.py:65: elif not opts.get('title') and opts.get('change'): Nit: `and opts.get('change')` is unnecessary here. We are in the `else` case, so if --title isn't given then --change must be.
https://codereview.adblockplus.org/29371804/diff/29491604/hgreview.py File hgreview.py (right): https://codereview.adblockplus.org/29371804/diff/29491604/hgreview.py#newcode65 hgreview.py:65: elif not opts.get('title') and opts.get('change'): How? It could be that the submitter wants the prompt. So she would do `hg review -r tip -w ferris`. Without "and opts.get('change'), it would go in here and get a nullpointer.
https://codereview.adblockplus.org/29371804/diff/29491604/hgreview.py File hgreview.py (right): https://codereview.adblockplus.org/29371804/diff/29491604/hgreview.py#newcode65 hgreview.py:65: elif not opts.get('title') and opts.get('change'): On 2017/07/18 12:17:57, f.nicolaisen wrote: > How? It could be that the submitter wants the prompt. So she would do `hg review > -r tip -w ferris`. Without "and opts.get('change'), it would go in here and get > a nullpointer. No, we would get into the prompt case then. And since we went into the prompt, we cannot go in here any more (because of the `elif`).
https://codereview.adblockplus.org/29371804/diff/29491604/hgreview.py File hgreview.py (right): https://codereview.adblockplus.org/29371804/diff/29491604/hgreview.py#newcode65 hgreview.py:65: elif not opts.get('title') and opts.get('change'): On 2017/07/18 12:27:18, Wladimir Palant wrote: > On 2017/07/18 12:17:57, f.nicolaisen wrote: > > How? It could be that the submitter wants the prompt. So she would do `hg > review > > -r tip -w ferris`. Without "and opts.get('change'), it would go in here and > get > > a nullpointer. > > No, we would get into the prompt case then. And since we went into the prompt, > we cannot go in here any more (because of the `elif`). Acknowledged.
On 2017/07/18 13:46:39, f.nicolaisen wrote: > https://codereview.adblockplus.org/29371804/diff/29491604/hgreview.py > File hgreview.py (right): > > https://codereview.adblockplus.org/29371804/diff/29491604/hgreview.py#newcode65 > hgreview.py:65: elif not opts.get('title') and opts.get('change'): > On 2017/07/18 12:27:18, Wladimir Palant wrote: > > On 2017/07/18 12:17:57, f.nicolaisen wrote: > > > How? It could be that the submitter wants the prompt. So she would do `hg > > review > > > -r tip -w ferris`. Without "and opts.get('change'), it would go in here and > > get > > > a nullpointer. > > > > No, we would get into the prompt case then. And since we went into the prompt, > > we cannot go in here any more (because of the `elif`). > > Acknowledged. Fixed in PS6.
LGTM 
|
