search

google / git-appraise

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

All possible validation checks should be done before prompting the user to enter a message #21

Closed jishi9 closed 8 years ago

jishi9 commented 8 years ago

In comment.go the user may be prompted to enter a message via their configured text editor. The same issue applies to pull request #18

Command line argument checks (e.g. "You cannot combine the flags -lgtm and -nmw") and other checks (e.g. "There is no matching review") are currently performed after the user had already entered their commit message.

These checks should be done before prompting the user for a message. Otherwise the user will be prompted to enter their message again and again, whilst they get the command arguments right, or whilst they figure out what the cause of the error message is.

barbastan commented 8 years ago

Thanks for these comments!

Barbara

On Tue, Dec 22, 2015 at 3:06 PM, jishi9 notifications@github.com wrote:

In comment.go https://github.com/google/git-appraise/blob/master/commands/comment.go#L47 the user may be prompted to enter a message via their configured text editor. The same issue applies to pull request #18 https://github.com/google/git-appraise/pull/18

Command line argument checks (e.g. "You cannot combine the flags -lgtm and -nmw") and other checks (e.g. "There is no matching review") are currently performed after the user had already entered their commit message.

These checks should be done before prompting the user for a message. Otherwise the user will be prompted to enter their message again and again, whilst they get the command arguments right, or whilst they figure out what the cause of the error message is.

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

ojarjur commented 8 years ago

Thanks for catching this. I believe we've moved all of the relevant validation checks up before editing the message, but if we missed something, then please feel encouraged to reopen this issue.