From: Ben Keene Date: Thu, 05 Dec 2019 15:40:56 GMT Subject: Re: [PATCH v4 09/11] git-p4: Add usability enhancements Message-ID: <652370f7-d1c5-a78a-aa5f-6e0c3219cd36@gmail.com> In-Reply-To: On 12/5/2019 9:04 AM, Junio C Hamano wrote: > "Ben Keene via GitGitGadget" writes: > >> From: Ben Keene >> >> Issue: when prompting the user with raw_input, the tests are not forgiving of user input. For example, on the first query asks for a yes/no response. If the user enters the full word "yes" or "no" the test will fail. Additionally, offer the suggestion of setting git-p4.attemptRCSCleanup when applying a commit fails because of RCS keywords. Both of these changes are usability enhancement suggestions. > Drop "Issue: " and upcase "when" that follows. The rest of the > paragraph reads a lot better without it as a human friendly > description. > > "are usability enhancement suggestions"??? Leaves readers wonder > who suggested them, or you are suggesting but are willing the change > to be dropped, or what. Be a bit more assertive if you want to say > that you believe these two would improve usability. Thank you and I reworked my submissions. I'm moving them to a separate PR and will split the commit into 3 separate commits. >> Change the code prompting the user for input to sanitize the user input before checking the response by asking the response as a lower case string, trimming leading/trailing spaces, and returning the first character. >> >> Change the applyCommit() method that when applying a commit fails becasue of the P4 RCS Keywords, the user should consider setting git-p4.attemptRCSCleanup. > s/becasue/because/; > > I have a feeling that these two may be worth doing but are totally > separate issues, deserving two separate commits. Is there a good > reason why these two must go hand-in-hand? > Good idea, and I split them out. >> Signed-off-by: Ben Keene >> (cherry picked from commit 1fab571664f5b6ad4ef321199f52615a32a9f8c7) >> --- >> git-p4.py | 31 ++++++++++++++++++++++++++----- >> 1 file changed, 26 insertions(+), 5 deletions(-) >> >> diff --git a/git-p4.py b/git-p4.py >> index f7c0ef0c53..f13e4645a3 100755 >> --- a/git-p4.py >> +++ b/git-p4.py >> @@ -1909,7 +1909,8 @@ def edit_template(self, template_file): >> return True >> >> while True: >> - response = raw_input("Submit template unchanged. Submit anyway? [y]es, [n]o (skip this patch) ") >> + response = raw_input("Submit template unchanged. Submit anyway? [y]es, [n]o (skip this patch) ").lower() \ >> + .strip()[0] > You could have saved the patch by doing > > + .lower().strip()[0] > > instead, no? > > I wonder if it would be better to write a thin wrapper around raw_input() > that does the "downcase and take the first meaningful letter" thing > for you and call it prompt() or something like that. I created a new function prompt() as you suggested. >> @@ -4327,7 +4343,12 @@ def main(): >> description = cmd.description, >> formatter = HelpFormatter()) >> >> - (cmd, args) = parser.parse_args(sys.argv[2:], cmd); >> + try: >> + (cmd, args) = parser.parse_args(sys.argv[2:], cmd); >> + except: >> + parser.print_help() >> + raise >> + > This change may be a good idea to give help text when the command > line parsing fails, but a good change deserves to be explained. I > do not think I saw any mention of it in the proposed log message, > though. Yes, you're right.  I split this out into a separate commit as well and gave it a place or prominence. >> global verbose >> verbose = cmd.verbose >> if cmd.needsGit: