Re: [PATCH v4 09/11] git-p4: Add usability enhancements

> "Ben Keene via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> From: Ben Keene <seraphire@gmail.com>
>>
>> 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.

>> 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?
>

>> Signed-off-by: Ben Keene <seraphire@gmail.com>
>> (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.

>> @@ -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.