Added a reject subcommand #18
Conversation
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.
|
Thank you! Our Tech Lead will look at this after his return on Monday, Jan Barbara On Sun, Dec 20, 2015 at 3:04 AM, Harry Lawrence [email protected]
|
| var r *review.Review | ||
| var err error | ||
| if len(args) > 1 { | ||
| return errors.New("Only rejecting a single review is supported.") |
There was a problem hiding this comment.
Should this (and the other checks below) not be done before obtaining the reject message (which may possibly prompt the user to enter a message)?
Edited to avoid confusion
|
@jishi9 yes I think you're right, we could check 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! |
|
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 [email protected]
|
Yes I just realized this. I have created an issue #21 for that as well.
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 |
|
Hey @barbastan yeah this is basically just the same as @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. |
|
I was just questioning if the rejection message needed to be mandatory. Barbara On Tue, Dec 22, 2015 at 3:21 PM, Harry Lawrence [email protected]
|
|
@barbastan ahh okay I think I see what you mean. Yes you're absolutely right the reject is simply just a nmw. Originally, the 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! :) |
|
Yes, I heartily agree that a reject message with no info is mighty Yes, by "inline comments", I meant comments within the code -- specific Maybe, I'm unintentionally rude in my code reviews, but I rarely leave a Omar (our Tech Lead) will probably shoot me for this suggestion, but if Thanks! Barbara On Tue, Dec 22, 2015 at 3:36 PM, Harry Lawrence [email protected]
|
As suggested in google#21, various checks before a message is added to either the comment or rejection are made. This seems to make more sense.
|
@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? |
|
@hazbo yeah exactly. LGTM 👍 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. |
Agreed! 😄 Thanks for the feedback on this. |
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