On 12/5/2019 9:04 AM, Junio C Hamano wrote:
"Ben Keene via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes:
From: Ben Keene <seraphire@xxxxxxxxx>
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 <seraphire@xxxxxxxxx>
(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:
