Patchwork ui: use try..finally in configoverride

login
register
mail settings
Submitter Gregory Szorc
Date Nov. 26, 2016, 5:14 p.m.
Message ID <ace32ef8bb5a16683e03.1480180486@ubuntu-vm-main>
Download mbox | patch
Permalink /patch/17761/
State Accepted
Headers show

Comments

Gregory Szorc - Nov. 26, 2016, 5:14 p.m.
# HG changeset patch
# User Gregory Szorc <gregory.szorc@gmail.com>
# Date 1480180481 28800
#      Sat Nov 26 09:14:41 2016 -0800
# Node ID ace32ef8bb5a16683e0312d38e4ff5aa9abfc911
# Parent  906a7d8e969552536fffe0df7a5e63bf5d79b34b
ui: use try..finally in configoverride

@contextmanager almost always have their "yield" inside a try..finally
block. This is because if the calling code inside the activated
context manager raises, the code after the "yield" won't get
executed. A "finally" block, however, will get executed in this
scenario.
Jun Wu - Nov. 26, 2016, 11:32 p.m.
Looks good to me. Thanks for the cleanup! (I was thinking about adding a
review comment only to find it was pushed).

Excerpts from Gregory Szorc's message of 2016-11-26 09:14:46 -0800:
> # HG changeset patch
> # User Gregory Szorc <gregory.szorc@gmail.com>
> # Date 1480180481 28800
> #      Sat Nov 26 09:14:41 2016 -0800
> # Node ID ace32ef8bb5a16683e0312d38e4ff5aa9abfc911
> # Parent  906a7d8e969552536fffe0df7a5e63bf5d79b34b
> ui: use try..finally in configoverride
> 
> @contextmanager almost always have their "yield" inside a try..finally
> block. This is because if the calling code inside the activated
> context manager raises, the code after the "yield" won't get
> executed. A "finally" block, however, will get executed in this
> scenario.
> 
> diff --git a/mercurial/ui.py b/mercurial/ui.py
> --- a/mercurial/ui.py
> +++ b/mercurial/ui.py
> @@ -1201,16 +1201,18 @@ class ui(object):
>          `overrides` must be a dict of the following structure:
>          {(section, name) : value}"""
>          backups = {}
> -        for (section, name), value in overrides.items():
> -            backups[(section, name)] = self.backupconfig(section, name)
> -            self.setconfig(section, name, value, source)
> -        yield
> -        for __, backup in backups.items():
> -            self.restoreconfig(backup)
> -        # just restoring ui.quiet config to the previous value is not enough
> -        # as it does not update ui.quiet class member
> -        if ('ui', 'quiet') in overrides:
> -            self.fixconfig(section='ui')
> +        try:
> +            for (section, name), value in overrides.items():
> +                backups[(section, name)] = self.backupconfig(section, name)
> +                self.setconfig(section, name, value, source)
> +            yield
> +        finally:
> +            for __, backup in backups.items():
> +                self.restoreconfig(backup)
> +            # just restoring ui.quiet config to the previous value is not enough
> +            # as it does not update ui.quiet class member
> +            if ('ui', 'quiet') in overrides:
> +                self.fixconfig(section='ui')
>  
>  class paths(dict):
>      """Represents a collection of paths and their configs.
Yuya Nishihara - Nov. 27, 2016, 9:10 a.m.
On Sat, 26 Nov 2016 09:14:46 -0800, Gregory Szorc wrote:
> # HG changeset patch
> # User Gregory Szorc <gregory.szorc@gmail.com>
> # Date 1480180481 28800
> #      Sat Nov 26 09:14:41 2016 -0800
> # Node ID ace32ef8bb5a16683e0312d38e4ff5aa9abfc911
> # Parent  906a7d8e969552536fffe0df7a5e63bf5d79b34b
> ui: use try..finally in configoverride
> 
> @contextmanager almost always have their "yield" inside a try..finally
> block. This is because if the calling code inside the activated
> context manager raises, the code after the "yield" won't get
> executed. A "finally" block, however, will get executed in this
> scenario.

Sure, queued, thanks.
Kostia Balytskyi - Nov. 27, 2016, 1:52 p.m.
On 11/27/16, 9:10 AM, "Mercurial-devel on behalf of Yuya Nishihara" <mercurial-devel-bounces@mercurial-scm.org on behalf of yuya@tcha.org> wrote:

    On Sat, 26 Nov 2016 09:14:46 -0800, Gregory Szorc wrote:
    > # HG changeset patch

    > # User Gregory Szorc <gregory.szorc@gmail.com>

    > # Date 1480180481 28800

    > #      Sat Nov 26 09:14:41 2016 -0800

    > # Node ID ace32ef8bb5a16683e0312d38e4ff5aa9abfc911

    > # Parent  906a7d8e969552536fffe0df7a5e63bf5d79b34b

    > ui: use try..finally in configoverride

    > 

    > @contextmanager almost always have their "yield" inside a try..finally

    > block. This is because if the calling code inside the activated

    > context manager raises, the code after the "yield" won't get

    > executed. A "finally" block, however, will get executed in this

    > scenario.

    
    Sure, queued, thanks.

I did not know about this, should’ve read the docs more carefully. Thanks!
    _______________________________________________
    Mercurial-devel mailing list
    Mercurial-devel@mercurial-scm.org
    https://urldefense.proofpoint.com/v2/url?u=https-3A__www.mercurial-2Dscm.org_mailman_listinfo_mercurial-2Ddevel&d=DgIGaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=Pp-gQYFgs4tKlSFPF5kfCw&m=ovbkX-mbuGLQiRqmUixTyjQmk3Y5x_gRDg067rRv02s&s=7mpE6rQbWryTUWe0KfxuoVfGNmRz-dnxccmbpo6fIDM&e=

Patch

diff --git a/mercurial/ui.py b/mercurial/ui.py
--- a/mercurial/ui.py
+++ b/mercurial/ui.py
@@ -1201,16 +1201,18 @@  class ui(object):
         `overrides` must be a dict of the following structure:
         {(section, name) : value}"""
         backups = {}
-        for (section, name), value in overrides.items():
-            backups[(section, name)] = self.backupconfig(section, name)
-            self.setconfig(section, name, value, source)
-        yield
-        for __, backup in backups.items():
-            self.restoreconfig(backup)
-        # just restoring ui.quiet config to the previous value is not enough
-        # as it does not update ui.quiet class member
-        if ('ui', 'quiet') in overrides:
-            self.fixconfig(section='ui')
+        try:
+            for (section, name), value in overrides.items():
+                backups[(section, name)] = self.backupconfig(section, name)
+                self.setconfig(section, name, value, source)
+            yield
+        finally:
+            for __, backup in backups.items():
+                self.restoreconfig(backup)
+            # just restoring ui.quiet config to the previous value is not enough
+            # as it does not update ui.quiet class member
+            if ('ui', 'quiet') in overrides:
+                self.fixconfig(section='ui')
 
 class paths(dict):
     """Represents a collection of paths and their configs.