diff mbox series

[4/6] patman: Make most bool arguments BooleanOptionalAction

Message ID 20220630140742.4.I6cb1a37e190dd759acc478beb2b0e0dc8e483923@changeid
State Superseded
Delegated to: Simon Glass
Headers show
Series patman: Small fixes plus remove --no-tree from checkpatch for linux | expand

Commit Message

Doug Anderson June 30, 2022, 9:08 p.m. UTC
For boolean arguments it's convenient to be able to specify both the
argument and its opposite on the command line. This is especially
convenient because you can change the default via the settings file
and being able express the opposite can be the only way to override
things.

Luckily python handles this well--we just need to specify things with
BooleanOptionalAction. We'll do that for all options except
"full-help" (where it feels silly). This uglifies the help text a
little bit but does give maximum flexibility.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

 tools/patman/main.py | 52 +++++++++++++++++++++++---------------------
 1 file changed, 27 insertions(+), 25 deletions(-)

Comments

Brian Norris June 30, 2022, 9:30 p.m. UTC | #1
On Thu, Jun 30, 2022 at 02:08:07PM -0700, Doug Anderson wrote:
> For boolean arguments it's convenient to be able to specify both the
> argument and its opposite on the command line. This is especially
> convenient because you can change the default via the settings file
> and being able express the opposite can be the only way to override
> things.
> 
> Luckily python handles this well--we just need to specify things with
> BooleanOptionalAction. We'll do that for all options except
> "full-help" (where it feels silly). This uglifies the help text a
> little bit but does give maximum flexibility.
> 
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
> 
>  tools/patman/main.py | 52 +++++++++++++++++++++++---------------------
>  1 file changed, 27 insertions(+), 25 deletions(-)
> 
> diff --git a/tools/patman/main.py b/tools/patman/main.py
> index 336f4e439aa9..9684300c022c 100755
> --- a/tools/patman/main.py
> +++ b/tools/patman/main.py

> -send.add_argument('-t', '--ignore-bad-tags', action='store_true',
> +send.add_argument('-t', '--ignore-bad-tags', action=BooleanOptionalAction,
>                    default=False,
>                    help='Ignore bad tags / aliases (default=warn)')

I know you mentioned --help ugliness, but this one ends up looking like:

  (default=warn) (default: False)

Perhaps we should drop the baked-in "(default=warn)" text?

Otherwise:

Reviewed-by: Brian Norris <briannorris@chromium.org>
Tested-by: Brian Norris <briannorris@chromium.org>
diff mbox series

Patch

diff --git a/tools/patman/main.py b/tools/patman/main.py
index 336f4e439aa9..9684300c022c 100755
--- a/tools/patman/main.py
+++ b/tools/patman/main.py
@@ -6,7 +6,7 @@ 
 
 """See README for more information"""
 
-from argparse import ArgumentParser
+from argparse import ArgumentParser, BooleanOptionalAction
 import os
 import re
 import shutil
@@ -40,7 +40,7 @@  parser.add_argument('-c', '--count', dest='count', type=int,
     default=-1, help='Automatically create patches from top n commits')
 parser.add_argument('-e', '--end', type=int, default=0,
     help='Commits to skip at end of patch list')
-parser.add_argument('-D', '--debug', action='store_true',
+parser.add_argument('-D', '--debug', action=BooleanOptionalAction,
     help='Enabling debugging (provides a full traceback on error)')
 parser.add_argument('-p', '--project', default=project.detect_project(),
                     help="Project name; affects default option values and "
@@ -50,42 +50,44 @@  parser.add_argument('-P', '--patchwork-url',
                     help='URL of patchwork server [default: %(default)s]')
 parser.add_argument('-s', '--start', dest='start', type=int,
     default=0, help='Commit to start creating patches from (0 = HEAD)')
-parser.add_argument('-v', '--verbose', action='store_true', dest='verbose',
-                    default=False, help='Verbose output of errors and warnings')
+parser.add_argument('-v', '--verbose', action=BooleanOptionalAction,
+                    dest='verbose', default=False,
+                    help='Verbose output of errors and warnings')
 parser.add_argument('-H', '--full-help', action='store_true', dest='full_help',
                     default=False, help='Display the README file')
 
 subparsers = parser.add_subparsers(dest='cmd')
 send = subparsers.add_parser('send')
-send.add_argument('-i', '--ignore-errors', action='store_true',
-       dest='ignore_errors', default=False,
-       help='Send patches email even if patch errors are found')
+send.add_argument('-i', '--ignore-errors', action=BooleanOptionalAction,
+                  dest='ignore_errors', default=False,
+                  help='Send patches email even if patch errors are found')
 send.add_argument('-l', '--limit-cc', dest='limit', type=int, default=None,
        help='Limit the cc list to LIMIT entries [default: %(default)s]')
-send.add_argument('-m', '--no-maintainers', action='store_false',
-       dest='add_maintainers', default=True,
-       help="Don't cc the file maintainers automatically")
-send.add_argument('-n', '--dry-run', action='store_true', dest='dry_run',
-       default=False, help="Do a dry run (create but don't email patches)")
+send.add_argument('-m', '--maintainers', action=BooleanOptionalAction,
+                  dest='add_maintainers', default=True,
+                  help="cc the file maintainers automatically")
+send.add_argument('-n', '--dry-run', action=BooleanOptionalAction,
+                  dest='dry_run', default=False,
+                  help="Do a dry run (create but don't email patches)")
 send.add_argument('-r', '--in-reply-to', type=str, action='store',
                   help="Message ID that this series is in reply to")
-send.add_argument('-t', '--ignore-bad-tags', action='store_true',
+send.add_argument('-t', '--ignore-bad-tags', action=BooleanOptionalAction,
                   default=False,
                   help='Ignore bad tags / aliases (default=warn)')
-send.add_argument('-T', '--thread', action='store_true', dest='thread',
+send.add_argument('-T', '--thread', action=BooleanOptionalAction, dest='thread',
                   default=False, help='Create patches as a single thread')
 send.add_argument('--cc-cmd', dest='cc_cmd', type=str, action='store',
        default=None, help='Output cc list for patch file (used by git)')
-send.add_argument('--no-binary', action='store_true', dest='ignore_binary',
-                  default=False,
-                  help="Do not output contents of changes in binary files")
-send.add_argument('--no-check', action='store_false', dest='check_patch',
+send.add_argument('--binary', action=BooleanOptionalAction,
+                  dest='ignore_binary', default=False,
+                  help="Output contents of changes in binary files")
+send.add_argument('--check', action=BooleanOptionalAction, dest='check_patch',
                   default=True,
-                  help="Don't check for patch compliance")
-send.add_argument('--no-tags', action='store_false', dest='process_tags',
-                  default=True, help="Don't process subject tags as aliases")
-send.add_argument('--no-signoff', action='store_false', dest='add_signoff',
-                  default=True, help="Don't add Signed-off-by to patches")
+                  help="Check for patch compliance")
+send.add_argument('--tags', action=BooleanOptionalAction, dest='process_tags',
+                  default=True, help="Process subject tags as aliases")
+send.add_argument('--signoff', action=BooleanOptionalAction, dest='add_signoff',
+                  default=True, help="Add Signed-off-by to patches")
 send.add_argument('--smtp-server', type=str,
                   help="Specify the SMTP server to 'git send-email'")
 
@@ -97,11 +99,11 @@  test_parser.add_argument('testname', type=str, default=None, nargs='?',
 
 status = subparsers.add_parser('status',
                                help='Check status of patches in patchwork')
-status.add_argument('-C', '--show-comments', action='store_true',
+status.add_argument('-C', '--show-comments', action=BooleanOptionalAction,
                     help='Show comments from each patch')
 status.add_argument('-d', '--dest-branch', type=str,
                     help='Name of branch to create with collected responses')
-status.add_argument('-f', '--force', action='store_true',
+status.add_argument('-f', '--force', action=BooleanOptionalAction,
                     help='Force overwriting an existing branch')
 
 # Parse options twice: first to get the project and second to handle