Patchwork [RFC] ui: add configoverride context manager

login
register
mail settings
Submitter Kostia Balytskyi
Date Nov. 20, 2016, 4:13 p.m.
Message ID <b226d57bafe182f379b1.1479658389@dev1902.lla1.facebook.com>
Download mbox | patch
Permalink /patch/17651/
State Superseded
Headers show

Comments

Kostia Balytskyi - Nov. 20, 2016, 4:13 p.m.
# HG changeset patch
# User Kostia Balytskyi <ikostia@fb.com>
# Date 1479658212 28800
#      Sun Nov 20 08:10:12 2016 -0800
# Node ID b226d57bafe182f379b144ec4f6374fedfc124ef
# Parent  141b0d27e9e1e846215ead5314237536efc1a185
ui: add configoverride context manager

I feel like this idea might've been discussed before, so please
feel free to point me to the right mailing list entry to read
about why it should not be done.

We have a common pattern of the following code:
    backup = ui.backupconfig(section, name)
    try:
        ui.setconfig(section, name, temporaryvalue, source)
        do_something()
    finally:
        ui.restoreconfig(backup)

IMO, this looks better:
    with ui.configoverride([(section, name, temporaryvalue)], source):
        do_something()

Especially this becomes more convenient when one has to backup multiple
config values before doing something. In such case, adding a new value
to backup requires codemod in three places.

Note that this contextmanager does not have to be a member ui class,
but it felt like the right place to me.

I have tested it manually, but I am not sure how an automated test
can be written here.
Augie Fackler - Nov. 21, 2016, 8:20 p.m.
On Sun, Nov 20, 2016 at 08:13:09AM -0800, Kostia Balytskyi wrote:
> # HG changeset patch
> # User Kostia Balytskyi <ikostia@fb.com>
> # Date 1479658212 28800
> #      Sun Nov 20 08:10:12 2016 -0800
> # Node ID b226d57bafe182f379b144ec4f6374fedfc124ef
> # Parent  141b0d27e9e1e846215ead5314237536efc1a185
> ui: add configoverride context manager
>
> I feel like this idea might've been discussed before, so please
> feel free to point me to the right mailing list entry to read
> about why it should not be done.
>
> We have a common pattern of the following code:
>     backup = ui.backupconfig(section, name)
>     try:
>         ui.setconfig(section, name, temporaryvalue, source)
>         do_something()
>     finally:
>         ui.restoreconfig(backup)
>
> IMO, this looks better:
>     with ui.configoverride([(section, name, temporaryvalue)], source):

What do you think about making this a dict instead of a list of tuples? That is:

with ui.configoverride({(section, name): temporaryvalue}, source):

That feels slightly less error-prone to me.

>         do_something()
>
> Especially this becomes more convenient when one has to backup multiple
> config values before doing something. In such case, adding a new value
> to backup requires codemod in three places.
>
> Note that this contextmanager does not have to be a member ui class,
> but it felt like the right place to me.
>
> I have tested it manually, but I am not sure how an automated test
> can be written here.
>
> diff --git a/mercurial/ui.py b/mercurial/ui.py
> --- a/mercurial/ui.py
> +++ b/mercurial/ui.py
> @@ -7,6 +7,7 @@
>
>  from __future__ import absolute_import
>
> +import contextlib
>  import errno
>  import getpass
>  import inspect
> @@ -1193,6 +1194,26 @@ class ui(object):
>                  " update your code.)") % version
>          self.develwarn(msg, stacklevel=2, config='deprec-warn')
>
> +    @contextlib.contextmanager
> +    def configoverride(self, overrides, source="", quiet=None):

Since we're making a new API, can we make source mandatory?

Also, why the special handling for quiet? Is it overridden that often
without overriding other things?

> +        """Context manager for temporary config overrides
> +        `overrides` must be a list of three-element tuples:
> +        [(section, name, value)].
> +        `quiet` is an optional override for `ui.quiet`"""
> +        backups = {}
> +        for override in overrides:
> +            section, name, value = override
> +            backups[(section, name)] = self.backupconfig(section, name)
> +            self.setconfig(section, name, value, source)
> +        if quiet is not None:
> +            backupquiet = self.quiet
> +            self.quiet = quiet
> +        yield
> +        for __, backup in backups.items():
> +            self.restoreconfig(backup)
> +        if quiet is not None:
> +            self.quiet = backupquiet
> +
>  class paths(dict):
>      """Represents a collection of paths and their configs.
>
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

Patch

diff --git a/mercurial/ui.py b/mercurial/ui.py
--- a/mercurial/ui.py
+++ b/mercurial/ui.py
@@ -7,6 +7,7 @@ 
 
 from __future__ import absolute_import
 
+import contextlib
 import errno
 import getpass
 import inspect
@@ -1193,6 +1194,26 @@  class ui(object):
                 " update your code.)") % version
         self.develwarn(msg, stacklevel=2, config='deprec-warn')
 
+    @contextlib.contextmanager
+    def configoverride(self, overrides, source="", quiet=None):
+        """Context manager for temporary config overrides
+        `overrides` must be a list of three-element tuples:
+        [(section, name, value)].
+        `quiet` is an optional override for `ui.quiet`"""
+        backups = {}
+        for override in overrides:
+            section, name, value = override
+            backups[(section, name)] = self.backupconfig(section, name)
+            self.setconfig(section, name, value, source)
+        if quiet is not None:
+            backupquiet = self.quiet
+            self.quiet = quiet
+        yield
+        for __, backup in backups.items():
+            self.restoreconfig(backup)
+        if quiet is not None:
+            self.quiet = backupquiet
+
 class paths(dict):
     """Represents a collection of paths and their configs.