From bbf6f2771532fd61e09db93a7fca0713d04a91ce Mon Sep 17 00:00:00 2001 From: Dan Helfman Date: Wed, 2 Apr 2025 17:08:04 -0700 Subject: [PATCH] For boolean configuration options, add separate "--foo" and "--no-foo" CLI flags (#303). --- borgmatic/commands/arguments.py | 21 +++--- borgmatic/commands/borgmatic.py | 2 +- borgmatic/config/schema.yaml | 36 +---------- borgmatic/logger.py | 9 +-- tests/unit/commands/test_arguments.py | 93 ++++++++++++--------------- tests/unit/test_logger.py | 42 +++++++----- 6 files changed, 86 insertions(+), 117 deletions(-) diff --git a/borgmatic/commands/arguments.py b/borgmatic/commands/arguments.py index 9c896c34..2e71ed1d 100644 --- a/borgmatic/commands/arguments.py +++ b/borgmatic/commands/arguments.py @@ -500,20 +500,26 @@ def add_arguments_from_schema(arguments_group, schema, unparsed_arguments, names # The ...=str given here is to support specifying an object or an array as a YAML string on the # command-line. argument_type = borgmatic.config.schema.parse_type(schema_type, object=str, array=str) - full_flag_name = f"--{flag_name}" - # As a UX nicety, allow boolean options that have a default of false to have command-line flags - # without values. - if schema_type == 'boolean' and schema.get('default') is False: + # As a UX nicety, add separate true and false flags for boolean options. + if schema_type == 'boolean': arguments_group.add_argument( - full_flag_name, + f'--{flag_name}', action='store_true', default=None, help=description, ) + no_flag_name = '.'.join(names[:-1] + ('no-' + names[-1],)).replace('_', '-') + arguments_group.add_argument( + f'--{no_flag_name}', + dest=flag_name.replace('-', '_'), + action='store_false', + default=None, + help=f'Set the --{flag_name} value to false.', + ) else: arguments_group.add_argument( - full_flag_name, + f'--{flag_name}', type=argument_type, metavar=metavar, help=description, @@ -553,9 +559,6 @@ def make_parsers(schema, unparsed_arguments): action='store_true', help='Go through the motions, but do not actually write to any repositories', ) - global_group.add_argument( - '-nc', '--no-color', dest='no_color', action='store_true', help='Disable colored output' - ) global_group.add_argument( '-v', '--verbosity', diff --git a/borgmatic/commands/borgmatic.py b/borgmatic/commands/borgmatic.py index 097fe488..5bf41db0 100644 --- a/borgmatic/commands/borgmatic.py +++ b/borgmatic/commands/borgmatic.py @@ -1025,7 +1025,7 @@ def main(extra_summary_logs=[]): # pragma: no cover any_json_flags = any( getattr(sub_arguments, 'json', False) for sub_arguments in arguments.values() ) - color_enabled = should_do_markup(global_arguments.no_color or any_json_flags, configs) + color_enabled = should_do_markup(configs, any_json_flags) try: configure_logging( diff --git a/borgmatic/config/schema.yaml b/borgmatic/config/schema.yaml index d5c572f0..87388735 100644 --- a/borgmatic/config/schema.yaml +++ b/borgmatic/config/schema.yaml @@ -59,7 +59,6 @@ properties: description: | Whether the repository should be created append-only, only used for the repo-create action. Defaults to false. - default: false example: true storage_quota: type: string @@ -74,7 +73,6 @@ properties: Whether any missing parent directories of the repository path should be created, only used for the repo-create action. Defaults to false. - default: false example: true description: | A required list of local or remote repositories with paths and @@ -104,14 +102,12 @@ properties: description: | Stay in same file system; do not cross mount points beyond the given source directories. Defaults to false. - default: false example: true numeric_ids: type: boolean description: | Only store/extract numeric user and group identifiers. Defaults to false. - default: false example: true atime: type: boolean @@ -122,13 +118,11 @@ properties: ctime: type: boolean description: Store ctime into archive. Defaults to true. - default: true example: false birthtime: type: boolean description: | Store birthtime (creation date) into archive. Defaults to true. - default: true example: false read_special: type: boolean @@ -138,14 +132,12 @@ properties: used when backing up special devices such as /dev/zero. Defaults to false. But when a database hook is used, the setting here is ignored and read_special is considered true. - default: false example: true flags: type: boolean description: | Record filesystem flags (e.g. NODUMP, IMMUTABLE) in archive. Defaults to true. - default: true example: false files_cache: type: string @@ -219,7 +211,6 @@ properties: Exclude directories that contain a CACHEDIR.TAG file. See http://www.brynosaurus.com/cachedir/spec.html for details. Defaults to false. - default: false example: true exclude_if_present: type: array @@ -236,13 +227,11 @@ properties: If true, the exclude_if_present filename is included in backups. Defaults to false, meaning that the exclude_if_present filename is omitted from backups. - default: false example: true exclude_nodump: type: boolean description: | Exclude files with the NODUMP flag. Defaults to false. - default: false example: true borgmatic_source_directory: type: string @@ -274,7 +263,6 @@ properties: description: | If true, then source directories (and root pattern paths) must exist. If they don't, an error is raised. Defaults to false. - default: false example: true encryption_passcommand: type: string @@ -488,21 +476,18 @@ properties: description: | Bypass Borg error about a repository that has been moved. Defaults to false. - default: false example: true unknown_unencrypted_repo_access_is_ok: type: boolean description: | Bypass Borg error about a previously unknown unencrypted repository. Defaults to false. - default: false example: true check_i_know_what_i_am_doing: type: boolean description: | Bypass Borg confirmation about check with repair option. Defaults to false and an interactive prompt from Borg. - default: false example: true extra_borg_options: type: object @@ -828,9 +813,7 @@ properties: color: type: boolean description: | - Apply color to console output. Can be overridden with --no-color - command-line flag. Defaults to true. - default: true + Apply color to console output. Defaults to true. example: false progress: type: boolean @@ -838,7 +821,6 @@ properties: Display progress as each file or archive is processed when running supported actions. Corresponds to the "--progress" flag on those actions. Defaults to false. - default: false example: true statistics: type: boolean @@ -846,7 +828,6 @@ properties: Display statistics for an archive when running supported actions. Corresponds to the "--stats" flag on those actions. Defaults to false. - default: false example: true list_details: type: boolean @@ -854,7 +835,6 @@ properties: Display details for each file or archive as it is processed when running supported actions. Corresponds to the "--list" flag on those actions. Defaults to false. - default: false example: true skip_actions: type: array @@ -1197,7 +1177,6 @@ properties: backup itself. Defaults to true. Changing this to false prevents "borgmatic bootstrap" from extracting configuration files from the backup. - default: true example: false description: | Support for the "borgmatic bootstrap" action, used to extract @@ -1282,7 +1261,6 @@ properties: schema elements. These statements will fail unless the initial connection to the database is made by a superuser. - default: false example: true format: type: string @@ -1521,7 +1499,6 @@ properties: Use the "--add-drop-database" flag with mariadb-dump, causing the database to be dropped right before restore. Defaults to true. - default: true example: false options: type: string @@ -1669,7 +1646,6 @@ properties: Use the "--add-drop-database" flag with mysqldump, causing the database to be dropped right before restore. Defaults to true. - default: true example: false options: type: string @@ -2397,9 +2373,8 @@ properties: send_logs: type: boolean description: | - Send borgmatic logs to Apprise services as part the + Send borgmatic logs to Apprise services as part of the "finish", "fail", and "log" states. Defaults to true. - default: true example: false logs_size_limit: type: integer @@ -2509,14 +2484,12 @@ properties: description: | Verify the TLS certificate of the ping URL host. Defaults to true. - default: true example: false send_logs: type: boolean description: | - Send borgmatic logs to Healthchecks as part the "finish", + Send borgmatic logs to Healthchecks as part of the "finish", "fail", and "log" states. Defaults to true. - default: true example: false ping_body_limit: type: integer @@ -2549,7 +2522,6 @@ properties: the slug URL scheme (https://hc-ping.com// as opposed to https://hc-ping.com/). Defaults to false. - default: false example: true description: | Configuration for a monitoring integration with Healthchecks. Create @@ -2589,7 +2561,6 @@ properties: description: | Verify the TLS certificate of the push URL host. Defaults to true. - default: true example: false description: | Configuration for a monitoring integration with Uptime Kuma using @@ -2626,7 +2597,6 @@ properties: description: | Send borgmatic logs to PagerDuty when a backup errors. Defaults to true. - default: true example: false description: | Configuration for a monitoring integration with PagerDuty. Create an diff --git a/borgmatic/logger.py b/borgmatic/logger.py index 8e327b62..4eb34a34 100644 --- a/borgmatic/logger.py +++ b/borgmatic/logger.py @@ -29,12 +29,13 @@ def interactive_console(): return sys.stderr.isatty() and os.environ.get('TERM') != 'dumb' -def should_do_markup(no_color, configs): +def should_do_markup(configs, json_enabled): ''' - Given the value of the command-line no-color argument, and a dict of configuration filename to - corresponding parsed configuration, determine if we should enable color marking up. + Given a dict of configuration filename to corresponding parsed configuration (which already have + any command-line overrides applied) and whether json is enabled, determine if we should enable + color marking up. ''' - if no_color: + if json_enabled: return False if any(config.get('color', True) is False for config in configs.values()): diff --git a/tests/unit/commands/test_arguments.py b/tests/unit/commands/test_arguments.py index 1f2457dc..51a64761 100644 --- a/tests/unit/commands/test_arguments.py +++ b/tests/unit/commands/test_arguments.py @@ -1121,7 +1121,7 @@ def test_add_arguments_from_schema_with_array_and_nested_object_adds_multiple_fl ) -def test_add_arguments_from_schema_with_default_false_boolean_adds_valueless_flag(): +def test_add_arguments_from_schema_with_boolean_adds_two_valueless_flags(): arguments_group = flexmock() flexmock(module).should_receive('make_argument_description').and_return('help') flexmock(module.borgmatic.config.schema).should_receive('parse_type').and_return(bool) @@ -1131,59 +1131,12 @@ def test_add_arguments_from_schema_with_default_false_boolean_adds_valueless_fla default=None, help='help', ).once() - flexmock(module).should_receive('add_array_element_arguments') - - module.add_arguments_from_schema( - arguments_group=arguments_group, - schema={ - 'type': 'object', - 'properties': { - 'foo': { - 'type': 'boolean', - 'default': False, - } - }, - }, - unparsed_arguments=(), - ) - - -def test_add_arguments_from_schema_with_default_true_boolean_adds_value_flag(): - arguments_group = flexmock() - flexmock(module).should_receive('make_argument_description').and_return('help') - flexmock(module.borgmatic.config.schema).should_receive('parse_type').and_return(bool) arguments_group.should_receive('add_argument').with_args( - '--foo', - type=bool, - metavar='FOO', - help='help', - ).once() - flexmock(module).should_receive('add_array_element_arguments') - - module.add_arguments_from_schema( - arguments_group=arguments_group, - schema={ - 'type': 'object', - 'properties': { - 'foo': { - 'type': 'boolean', - 'default': True, - } - }, - }, - unparsed_arguments=(), - ) - - -def test_add_arguments_from_schema_with_defaultless_boolean_adds_value_flag(): - arguments_group = flexmock() - flexmock(module).should_receive('make_argument_description').and_return('help') - flexmock(module.borgmatic.config.schema).should_receive('parse_type').and_return(bool) - arguments_group.should_receive('add_argument').with_args( - '--foo', - type=bool, - metavar='FOO', - help='help', + '--no-foo', + dest='foo', + action='store_false', + default=None, + help=object, ).once() flexmock(module).should_receive('add_array_element_arguments') @@ -1201,6 +1154,40 @@ def test_add_arguments_from_schema_with_defaultless_boolean_adds_value_flag(): ) +def test_add_arguments_from_schema_with_nested_boolean_adds_two_valueless_flags(): + arguments_group = flexmock() + flexmock(module).should_receive('make_argument_description').and_return('help') + flexmock(module.borgmatic.config.schema).should_receive('parse_type').and_return(bool) + arguments_group.should_receive('add_argument').with_args( + '--foo.bar.baz-quux', + action='store_true', + default=None, + help='help', + ).once() + arguments_group.should_receive('add_argument').with_args( + '--foo.bar.no-baz-quux', + dest='foo.bar.baz_quux', + action='store_false', + default=None, + help=object, + ).once() + flexmock(module).should_receive('add_array_element_arguments') + + module.add_arguments_from_schema( + arguments_group=arguments_group, + schema={ + 'type': 'object', + 'properties': { + 'baz_quux': { + 'type': 'boolean', + } + }, + }, + unparsed_arguments=(), + names=('foo', 'bar'), + ) + + def test_add_arguments_from_schema_skips_omitted_flag_name(): arguments_group = flexmock() flexmock(module).should_receive('make_argument_description').and_return('help') diff --git a/tests/unit/test_logger.py b/tests/unit/test_logger.py index dffaafc0..d179735f 100644 --- a/tests/unit/test_logger.py +++ b/tests/unit/test_logger.py @@ -44,19 +44,23 @@ def test_interactive_console_true_when_isatty_and_TERM_is_not_dumb(capsys): assert module.interactive_console() is True -def test_should_do_markup_respects_no_color_value(): - flexmock(module.os.environ).should_receive('get').and_return(None) +def test_should_do_markup_respects_json_enabled_value(): + flexmock(module.os.environ).should_receive('get').never() flexmock(module).should_receive('interactive_console').never() - assert module.should_do_markup(no_color=True, configs={}) is False + assert module.should_do_markup(configs={}, json_enabled=True) is False def test_should_do_markup_respects_config_value(): flexmock(module.os.environ).should_receive('get').and_return(None) flexmock(module).should_receive('interactive_console').never() - assert module.should_do_markup(no_color=False, configs={'foo.yaml': {'color': False}}) is False + assert ( + module.should_do_markup(configs={'foo.yaml': {'color': False}}, json_enabled=False) is False + ) flexmock(module).should_receive('interactive_console').and_return(True).once() - assert module.should_do_markup(no_color=False, configs={'foo.yaml': {'color': True}}) is True + assert ( + module.should_do_markup(configs={'foo.yaml': {'color': True}}, json_enabled=False) is True + ) def test_should_do_markup_prefers_any_false_config_value(): @@ -65,11 +69,11 @@ def test_should_do_markup_prefers_any_false_config_value(): assert ( module.should_do_markup( - no_color=False, configs={ 'foo.yaml': {'color': True}, 'bar.yaml': {'color': False}, }, + json_enabled=False, ) is False ) @@ -83,14 +87,16 @@ def test_should_do_markup_respects_PY_COLORS_environment_variable(): flexmock(module).should_receive('to_bool').and_return(True) - assert module.should_do_markup(no_color=False, configs={}) is True + assert module.should_do_markup(configs={}, json_enabled=False) is True -def test_should_do_markup_prefers_no_color_value_to_config_value(): +def test_should_do_markup_prefers_json_enabled_value_to_config_value(): flexmock(module.os.environ).should_receive('get').and_return(None) flexmock(module).should_receive('interactive_console').never() - assert module.should_do_markup(no_color=True, configs={'foo.yaml': {'color': True}}) is False + assert ( + module.should_do_markup(configs={'foo.yaml': {'color': True}}, json_enabled=True) is False + ) def test_should_do_markup_prefers_config_value_to_environment_variables(): @@ -98,7 +104,9 @@ def test_should_do_markup_prefers_config_value_to_environment_variables(): flexmock(module).should_receive('to_bool').and_return(True) flexmock(module).should_receive('interactive_console').never() - assert module.should_do_markup(no_color=False, configs={'foo.yaml': {'color': False}}) is False + assert ( + module.should_do_markup(configs={'foo.yaml': {'color': False}}, json_enabled=False) is False + ) def test_should_do_markup_prefers_no_color_value_to_environment_variables(): @@ -106,14 +114,14 @@ def test_should_do_markup_prefers_no_color_value_to_environment_variables(): flexmock(module).should_receive('to_bool').and_return(True) flexmock(module).should_receive('interactive_console').never() - assert module.should_do_markup(no_color=True, configs={}) is False + assert module.should_do_markup(configs={}, json_enabled=False) is False def test_should_do_markup_respects_interactive_console_value(): flexmock(module.os.environ).should_receive('get').and_return(None) flexmock(module).should_receive('interactive_console').and_return(True) - assert module.should_do_markup(no_color=False, configs={}) is True + assert module.should_do_markup(configs={}, json_enabled=False) is True def test_should_do_markup_prefers_PY_COLORS_to_interactive_console_value(): @@ -124,7 +132,7 @@ def test_should_do_markup_prefers_PY_COLORS_to_interactive_console_value(): flexmock(module).should_receive('to_bool').and_return(True) flexmock(module).should_receive('interactive_console').never() - assert module.should_do_markup(no_color=False, configs={}) is True + assert module.should_do_markup(configs={}, json_enabled=False) is True def test_should_do_markup_prefers_NO_COLOR_to_interactive_console_value(): @@ -132,7 +140,7 @@ def test_should_do_markup_prefers_NO_COLOR_to_interactive_console_value(): flexmock(module.os.environ).should_receive('get').with_args('NO_COLOR', None).and_return('True') flexmock(module).should_receive('interactive_console').never() - assert module.should_do_markup(no_color=False, configs={}) is False + assert module.should_do_markup(configs={}, json_enabled=False) is False def test_should_do_markup_respects_NO_COLOR_environment_variable(): @@ -140,7 +148,7 @@ def test_should_do_markup_respects_NO_COLOR_environment_variable(): flexmock(module.os.environ).should_receive('get').with_args('PY_COLORS', None).and_return(None) flexmock(module).should_receive('interactive_console').never() - assert module.should_do_markup(no_color=False, configs={}) is False + assert module.should_do_markup(configs={}, json_enabled=False) is False def test_should_do_markup_ignores_empty_NO_COLOR_environment_variable(): @@ -148,7 +156,7 @@ def test_should_do_markup_ignores_empty_NO_COLOR_environment_variable(): flexmock(module.os.environ).should_receive('get').with_args('PY_COLORS', None).and_return(None) flexmock(module).should_receive('interactive_console').and_return(True) - assert module.should_do_markup(no_color=False, configs={}) is True + assert module.should_do_markup(configs={}, json_enabled=False) is True def test_should_do_markup_prefers_NO_COLOR_to_PY_COLORS(): @@ -160,7 +168,7 @@ def test_should_do_markup_prefers_NO_COLOR_to_PY_COLORS(): ) flexmock(module).should_receive('interactive_console').never() - assert module.should_do_markup(no_color=False, configs={}) is False + assert module.should_do_markup(configs={}, json_enabled=False) is False def test_multi_stream_handler_logs_to_handler_for_log_level():