From b56a8905025d3469232daafc174a2c360e658da0 Mon Sep 17 00:00:00 2001 From: Michael Hanke Date: Wed, 20 Jul 2022 21:32:34 +0200 Subject: [PATCH 1/5] Support unsetting config via `datalad -c :` As documented in https://github.com/datalad/datalad/issues/5653#issuecomment-1190069481 this API was lacking a method to unset configuration items, while all other API do support this feature. This changeset adds it. Closes datalad/datalad#6863 --- datalad/cli/common_args.py | 11 +++++++---- datalad/cli/helpers.py | 14 ++++++++++---- datalad/cli/tests/test_main.py | 18 ++++++++++++++++++ 3 files changed, 35 insertions(+), 8 deletions(-) diff --git a/datalad/cli/common_args.py b/datalad/cli/common_args.py index 6b1d4b3f6c..704af8ae85 100644 --- a/datalad/cli/common_args.py +++ b/datalad/cli/common_args.py @@ -34,10 +34,13 @@ ('-c',), dict(action='append', dest='cfg_overrides', - metavar='KEY=VALUE', - help="""configuration variable setting. Overrides any - configuration read from a file, but is potentially overridden - itself by configuration variables in the process environment. + metavar='[:]name[=value]', + help="""specify configuration setting overrides. They override any + configuration read from a file. A configuration can also be + unset temporarily by prefixing its name with a colon (':') and + passing that as a value, e.g. ':user.name'. + Overrides specified here may be overriden themselves by + configuration settings declared as environment variables. """)), change_path=( ('-C',), diff --git a/datalad/cli/helpers.py b/datalad/cli/helpers.py index b657b65581..b20c467a48 100644 --- a/datalad/cli/helpers.py +++ b/datalad/cli/helpers.py @@ -255,17 +255,23 @@ def _parse_overrides_from_cmdline(cmdlineargs): # errors: we need a section, a variable, and a value at minimum # otherwise we break our own config parsing helpers # https://github.com/datalad/datalad/issues/3451 - noassign_expr = re.compile(r'[^\s]+\.[^\s]+=[\S]+') + assign_expr = re.compile(r'[^\s]+\.[^\s]+=[\S]+') + unset_expr = re.compile(r':[^\s]+\.[^\s=]+') noassign = [ o for o in cmdlineargs.cfg_overrides - if not noassign_expr.match(o) + if not (assign_expr.match(o) or unset_expr.match(o)) ] if noassign: lgr.error( "Configuration override without section/variable " - "or value assignment (must be 'section.variable=value'): %s", + "or unset marker or value assignment " + "(must be '[:]section.variable[=value]'): %s", noassign) sys.exit(3) - overrides = dict(o.split('=', 1) for o in cmdlineargs.cfg_overrides) + overrides = dict( + [o[1:], None] if o.startswith(':') + else o.split('=', 1) + for o in cmdlineargs.cfg_overrides + ) return overrides diff --git a/datalad/cli/tests/test_main.py b/datalad/cli/tests/test_main.py index 3cd55bc146..6cc4ef537e 100644 --- a/datalad/cli/tests/test_main.py +++ b/datalad/cli/tests/test_main.py @@ -305,6 +305,24 @@ def test_cfg_override(path=None): protocol=StdOutErrCapture)['stdout'] assert_in('datalad.dummy: this', out) + # set a config + run_main([ + 'configuration', '--scope', 'local', 'set', 'mike.item=some']) + # verify it is successfully set + assert 'some' == run_main([ + 'configuration', 'get', 'mike.item'])[0].strip() + # verify that an override can unset the config + # we need to use an ephemeral override dict, because otherwise + # run_main() would have the sideeffect of permanently modifying + # this session's global config manager + with patch('datalad.cfg.overrides', {}): + assert '' == run_main([ + '-c', ':mike.item', 'configuration', 'get', 'mike.item'])[0].strip() + # verify the effect is not permanent + assert 'some' == run_main([ + 'configuration', 'get', 'mike.item'])[0].strip() + + def test_incorrect_cfg_override(): run_main(['-c', 'some', 'wtf'], exit_code=3) From 7d1e2455d837d15889ef5d3ed368618bb29db6b6 Mon Sep 17 00:00:00 2001 From: Michael Hanke Date: Wed, 20 Jul 2022 21:35:14 +0200 Subject: [PATCH 2/5] Speed-up test execution Instead of running a complete `wtf` for just getting the config listed, limit to this particular section. This cuts the runtime in half down to 3.7s for the entire file (rather than the previously annotated 11s for this one test. --- datalad/cli/tests/test_main.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/datalad/cli/tests/test_main.py b/datalad/cli/tests/test_main.py index 6cc4ef537e..5e38fa4ea3 100644 --- a/datalad/cli/tests/test_main.py +++ b/datalad/cli/tests/test_main.py @@ -269,11 +269,12 @@ def test_script_shims(script): get_numeric_portion(version)) -@slow # 11.2591s @with_tempfile(mkdir=True) def test_cfg_override(path=None): with chpwd(path): - cmd = ['datalad', 'wtf', '-s', 'some'] + # use 'wtf' to dump the config + # should be rewritten to use `configuration` + cmd = ['datalad', 'wtf', '-S', 'configuration', '-s', 'some'] # control out = Runner().run(cmd, protocol=StdOutErrCapture)['stdout'] assert_not_in('datalad.dummy: this', out) From 40985d48125f038db83f1505e0eb5504bca5a24a Mon Sep 17 00:00:00 2001 From: Michael Hanke Date: Thu, 21 Jul 2022 07:06:16 +0200 Subject: [PATCH 3/5] Change formatting spec style As suggested by @yarikoptic. This adds `(...|...)` to ways of describing the concept of alternative values in the CLI. We already use `...|...` and `{..., ...}`. But given the absence of style guidelines, this change should be ok. Co-authored-by: Yaroslav Halchenko --- datalad/cli/common_args.py | 5 ++--- datalad/cli/helpers.py | 2 +- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/datalad/cli/common_args.py b/datalad/cli/common_args.py index 704af8ae85..985b7a4f77 100644 --- a/datalad/cli/common_args.py +++ b/datalad/cli/common_args.py @@ -34,11 +34,10 @@ ('-c',), dict(action='append', dest='cfg_overrides', - metavar='[:]name[=value]', + metavar='(:name|name=value)', help="""specify configuration setting overrides. They override any configuration read from a file. A configuration can also be - unset temporarily by prefixing its name with a colon (':') and - passing that as a value, e.g. ':user.name'. + unset temporarily by prefixing its name with a colon (':'), e.g. ':user.name'. Overrides specified here may be overriden themselves by configuration settings declared as environment variables. """)), diff --git a/datalad/cli/helpers.py b/datalad/cli/helpers.py index b20c467a48..c18691ac0a 100644 --- a/datalad/cli/helpers.py +++ b/datalad/cli/helpers.py @@ -266,7 +266,7 @@ def _parse_overrides_from_cmdline(cmdlineargs): lgr.error( "Configuration override without section/variable " "or unset marker or value assignment " - "(must be '[:]section.variable[=value]'): %s", + "(must be '(:section.variable|section.variable=value)'): %s", noassign) sys.exit(3) overrides = dict( From 2a8855a3cccbace713846fe22bc0a50cea611498 Mon Sep 17 00:00:00 2001 From: Michael Hanke Date: Thu, 21 Jul 2022 07:08:06 +0200 Subject: [PATCH 4/5] Fix typo --- datalad/cli/common_args.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/datalad/cli/common_args.py b/datalad/cli/common_args.py index 985b7a4f77..eeae343620 100644 --- a/datalad/cli/common_args.py +++ b/datalad/cli/common_args.py @@ -38,7 +38,7 @@ help="""specify configuration setting overrides. They override any configuration read from a file. A configuration can also be unset temporarily by prefixing its name with a colon (':'), e.g. ':user.name'. - Overrides specified here may be overriden themselves by + Overrides specified here may be overridden themselves by configuration settings declared as environment variables. """)), change_path=( From a556c2251e56a513b71d6c0247f7ab0b95cf5afb Mon Sep 17 00:00:00 2001 From: Michael Hanke Date: Thu, 21 Jul 2022 11:33:04 +0200 Subject: [PATCH 5/5] Test implementation did not consider side-effect of dataset instance lifetime --- datalad/cli/tests/test_main.py | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/datalad/cli/tests/test_main.py b/datalad/cli/tests/test_main.py index 5e38fa4ea3..5d5cba5f89 100644 --- a/datalad/cli/tests/test_main.py +++ b/datalad/cli/tests/test_main.py @@ -313,16 +313,19 @@ def test_cfg_override(path=None): assert 'some' == run_main([ 'configuration', 'get', 'mike.item'])[0].strip() # verify that an override can unset the config - # we need to use an ephemeral override dict, because otherwise - # run_main() would have the sideeffect of permanently modifying - # this session's global config manager - with patch('datalad.cfg.overrides', {}): - assert '' == run_main([ - '-c', ':mike.item', 'configuration', 'get', 'mike.item'])[0].strip() + # we cannot use run_main(), because the "singleton" instance of the + # dataset we are in is still around in this session, and with it + # also its config managers that we will not be able to post-hoc + # overwrite with this method. Instead, we'll execute in a subprocess. + assert '' == Runner().run([ + 'datalad', '-c', ':mike.item', + 'configuration', 'get', 'mike.item'], + protocol=StdOutErrCapture)['stdout'].strip() # verify the effect is not permanent - assert 'some' == run_main([ - 'configuration', 'get', 'mike.item'])[0].strip() - + assert 'some' == Runner().run([ + 'datalad', + 'configuration', 'get', 'mike.item'], + protocol=StdOutErrCapture)['stdout'].strip() def test_incorrect_cfg_override():