search

google / git-appraise

Distributed code review system for Git repos
Apache License 2.0
5.13k stars 146 forks source link

Added a reject subcommand #18

Closed hazbo closed 8 years ago

hazbo commented 8 years ago

The reject subcommand adds a NMW comment to the review. Unlike the accept subcommand, this requires a comment. The -m flag can be passed to do this, or if no flag is passed it will open the default git text editor.

This is what I have in mind in regards to #17

barbastan commented 8 years ago

Thank you! Our Tech Lead will look at this after his return on Monday, Jan 4th.

Barbara

On Sun, Dec 20, 2015 at 3:04 AM, Harry Lawrence notifications@github.com wrote:

The reject subcommand adds a NMW comment to the review. Unlike the accept subcommand, this requires a comment. The -m flag can be passed to do this, or if no flag is passed it will open the default git text editor.

This is what I have in mind in regards to #17

https://github.com/google/git-appraise/issues/17

You can view, comment on, or merge this pull request online at:

https://github.com/google/git-appraise/pull/18 Commit Summary

  • Added a reject subcommand

File Changes

Patch Links:

— Reply to this email directly or view it on GitHub https://github.com/google/git-appraise/pull/18.

hazbo commented 8 years ago

@jishi9 yes I think you're right, we could check len(args) (and other checks) before the editor code. As of writing this I just tried to keep it consistent with the editor code written in comment.go, but I understand where you're coming from.

When you say to prompt the user to write a message, could you explain a little more on what you mean by that? I think the message for a rejection should be mandatory simply due to the nature of the command itself.

Thanks!

barbastan commented 8 years ago

Maybe the inline code comments are adequate if this is equivalent to NMW?

Barbara

On Tue, Dec 22, 2015 at 2:51 PM, Harry Lawrence notifications@github.com wrote:

@jishi9 https://github.com/jishi9 yes I think you're right, we could check len(args) (and other checks) before the editor code. As of writing this I just tried to keep it consistent with the editor code written in comment.go, but I understand where you're coming from.

When you say to prompt the user to write a message, could you explain a little more on what you mean by that? I think the message for a rejection should be mandatory simply due to the nature of the command itself.

Thanks!

— Reply to this email directly or view it on GitHub https://github.com/google/git-appraise/pull/18#issuecomment-166755876.

jishi9 commented 8 years ago

@hazbo

As of writing this I just tried to keep it consistent with the editor code written in comment.go, but I understand where you're coming from.

Yes I just realized this. I have created an issue #21 for that as well.

When you say to prompt the user to write a message, could you explain a little more on what you mean by that? I think the message for a rejection should be mandatory simply due to the nature of the command itself.

Sorry what I said was confusing. I think you already understood what I meant: "we could check len(args) (and other checks) before the editor code."

You can strike out the (which may possibly prompt the user to enter a message) :)

hazbo commented 8 years ago

Hey @barbastan yeah this is basically just the same as git appraise comment -nmw -m 'Hey!' hash, could you explain for me a little more about what you mean? Sorry if I'm just being dumb and am missing the point.

@jishi9 okay that sounds great! I will look at making some amendments to both of these files in the morning. Thanks so much for pointing this out.

barbastan commented 8 years ago

I was just questioning if the rejection message needed to be mandatory. I'm assuming the reject == NMW. If there are inline comments that describe the additional work needed, might that not be adequate in many cases? Please argue if you disagree.

Barbara

On Tue, Dec 22, 2015 at 3:21 PM, Harry Lawrence notifications@github.com wrote:

Hey @barbastan https://github.com/barbastan yeah this is basically just the same as git appraise comment -nmw -m 'Hey!' hash, could you explain for me a little more about what you mean? Sorry if I'm just being dumb and am missing the point.

@jishi9 https://github.com/jishi9 okay that sounds great! I will look at making some amendments to both of these files in the morning. Thanks so much for pointing this out.

— Reply to this email directly or view it on GitHub https://github.com/google/git-appraise/pull/18#issuecomment-166760835.

hazbo commented 8 years ago

@barbastan ahh okay I think I see what you mean. Yes you're absolutely right the reject is simply just a nmw. Originally, the -m (or message) was not required for neither -lgtm or -nmw. @ojarjur and I had a brief discussion that led to us thinking that if someone is going to leave -nmw, some information on why that would be the case (in the form of a message) would be needed - as I'm sure you'd agree that it'd be rather unhelpful to leave this kind of negative feedback with no information related to it.

When you refer to inline comments, are you suggesting that these comments are left in the code suggesting feedback?

Thanks so much for weighing in on this! :)

barbastan commented 8 years ago

Yes, I heartily agree that a reject message with no info is mighty unhelpful.

Yes, by "inline comments", I meant comments within the code -- specific feedback with the code context.

Maybe, I'm unintentionally rude in my code reviews, but I rarely leave a top-level comment for NMW cases; I just leave comments within the code.

Omar (our Tech Lead) will probably shoot me for this suggestion, but if there are no inline comments, then a top-level comment should be required and only then. Hmmm... much easier just to require a top-level comment! :) I think I'm convinced.

Thanks!

Barbara

On Tue, Dec 22, 2015 at 3:36 PM, Harry Lawrence notifications@github.com wrote:

@barbastan https://github.com/barbastan ahh okay I think I see what you mean. Yes you're absolutely right the reject is simply just a nmw. Originally, the -m (or message) was not required for neither -lgtm or -nmw. @ojarjur https://github.com/ojarjur and I had a brief discussion https://github.com/google/git-appraise/issues/8#issuecomment-165078292 that led to us thinking that if someone is going to leave -nmw, some information on why that would be the case (in the form of a message) would be needed - as I'm sure you'd agree that it'd be rather unhelpful to leave this kind of negative feedback with no information related to it.

When you refer to inline comments, are you suggesting that these comments are left in the code suggesting feedback?

Thanks so much for weighing in on this! :)

— Reply to this email directly or view it on GitHub https://github.com/google/git-appraise/pull/18#issuecomment-166762577.

hazbo commented 8 years ago

@jishi9 I've made some changes here based on what you suggested, in regards to doing various checks. Is this what you had in mind?

jishi9 commented 8 years ago

@hazbo yeah exactly. LGTM :+1:

I just noticed that the message is actually persisted to a file, and hence the user would not have to re-type the message (I think). I still think it's better to do all validations first anyway.

hazbo commented 8 years ago

I still think it's better to do all validations first anyway.

Agreed! :smile:

Thanks for the feedback on this.