Patchwork [V4] ui: add new config flag for interface selection

login
register
mail settings
Submitter Laurent Charignon
Date Feb. 11, 2016, 5:14 p.m.
Message ID <53494a3e515b7744759d.1455210842@dev5073.prn1.facebook.com>
Download mbox | patch
Permalink /patch/13121/
State Superseded
Commit 71e12fc53b80180ca20e8192abf8ee4d70727fbe
Delegated to: Augie Fackler
Headers show

Comments

Laurent Charignon - Feb. 11, 2016, 5:14 p.m.
# HG changeset patch
# User Laurent Charignon <lcharignon@fb.com>
# Date 1455210657 28800
#      Thu Feb 11 09:10:57 2016 -0800
# Node ID 53494a3e515b7744759dfa064685b1b8299fffb7
# Parent  01a5143cd25f285f8c745a92986cd7186bb32c90
ui: add new config flag for interface selection

This patch introduces a new config flag ui.interface to select the interface
for interactive commands. It currently only applies to chunks selection.
The config can be overridden on a per feature basis with the flag
ui.interface.<feature>.

features for the moment can only be 'chunkselector', moving forward we expect
to have 'histedit' and other commands there.

If an incorrect value is given to ui.interface we print a warning and use the
default interface: text. If HGPLAIN is specified we also use the default
interface: text.
Pierre-Yves David - Feb. 12, 2016, 3:40 p.m.
On 02/11/2016 05:14 PM, Laurent Charignon wrote:
> # HG changeset patch
> # User Laurent Charignon <lcharignon@fb.com>
> # Date 1455210657 28800
> #      Thu Feb 11 09:10:57 2016 -0800
> # Node ID 53494a3e515b7744759dfa064685b1b8299fffb7
> # Parent  01a5143cd25f285f8c745a92986cd7186bb32c90
> ui: add new config flag for interface selection
>
> This patch introduces a new config flag ui.interface to select the interface
> for interactive commands. It currently only applies to chunks selection.
> The config can be overridden on a per feature basis with the flag
> ui.interface.<feature>.
>
> features for the moment can only be 'chunkselector', moving forward we expect
> to have 'histedit' and other commands there.
>
> If an incorrect value is given to ui.interface we print a warning and use the
> default interface: text. If HGPLAIN is specified we also use the default
> interface: text.

I think we should have a "auto" (name may varies) value, pick the 
currently best interface per subfeature. For example we already have a 
"curses" ui in storage for histedit, but is definitly not polished 
enough yet. We'll want to use curses for chunk selection as a default 
long before we might want curses ui for histedit.

That would be the default. Don't update your patch for that, we can 
follow up on that if people agree.

> diff --git a/mercurial/help/config.txt b/mercurial/help/config.txt
> --- a/mercurial/help/config.txt
> +++ b/mercurial/help/config.txt
> @@ -1587,6 +1587,17 @@
>   ``interactive``
>       Allow to prompt the user. (default: True)
>
> +``interface``
> +    Select the default interface for interactive features (default: text).
> +    Possible values are 'text' and 'curses'.
> +
> +``interface.chunkselector``
> +    Select the interface for chunkselection.

"override interactive interface choice for chunk selection"

I think we should also explain what is "chunk selection" here. Most 
people will not be able to link this with "hg commit -i".

Maybe "chunk selection" should be "patch edition" ? (very low confidence 
guess, juste hitting at the idea of having a different name).

> +    Possible values are 'text' and 'curses'.
> +    This config overrides the interface specified by ui.interface.
> +    If ui.interface and ui.interface.chunkselector aren't set, we use the text
> +    interface.

I think the last sentence is implies in the default of ui.interface and 
can be dropped.

>   ``logtemplate``
>       Template string for commands that print changesets.
>
> diff --git a/mercurial/ui.py b/mercurial/ui.py
> --- a/mercurial/ui.py
> +++ b/mercurial/ui.py
> @@ -699,6 +699,73 @@
>               return False
>           return util.isatty(fh)
>
> +    def interface(self, feature):
> +        '''what interface to use for interactive console features?

small nits: we usually use """ for docstring (not ''')

> +
> +        The interface is controlled by the value of `ui.interface` but also by
> +        the value of feature-specific configuration. For example:
> +
> +        ui.interface.histedit = text
> +        ui.interface.chunkselector = curses
> +
> +        Here the features are "histedit" and "chunkselector".
> +
> +        The configuration above means that the default interfaces for commands
> +        is curses, the interface for histedit is text and the interface for
> +        selecting chunk is crecord (the best curses interface available).
> +
> +        Consider the following exemple:
> +        ui.interface = curses
> +        ui.interface.histedit = text
> +
> +        Then histedit will use the text interface and chunkselector will use
> +        the default curses interface (crecord at the moment).
> +        '''
> +        if self.plain():
> +            return "text"
> +
> +        alloweddefaultinterfaces = ("text", "curses")
> +        defaultfeaturesinterfaces = {
> +            "chunkselector": {
> +                "text": "text",
> +                "curses": "crecord"

Why do we need to introduce a mention of crecord here? Could we do all 
this with "curses" and introduce the notion of value's hierarchy later?

small nits: verylowvariablenamestarttohurtreadability.

> +            }
> +        }
> +
> +        # Default interface for all the features
> +        defaultinterface = "text"
> +        i = self.config("ui", "interface", None)
> +        if i is not None:
> +            if i not in alloweddefaultinterfaces:
> +                self.warn(_("invalid value for ui.interface: %s\n") % i)
> +                self.warn(_("(using default interface instead %s)\n")
> +                          % defaultinterface)

We rarely do double line message. Could we fit this on a single line?

> +            else:
> +                defaultinterface = i

This is going to be quite confusing from a code flow perspective. You 
just move the value of a non-default interface into a variable called 
"defaultinterface"

I would use

   interface = self.config(…)
   if invalide(interface):
       warn()
       interface = defaultinterface

instead.

You can also get rid of the 'None' testing by using 'defaultinterface' 
as the fallback value of self.config(…)


> +        # Feature-specific interface
> +        if feature not in defaultfeaturesinterfaces.keys():
> +            # Programming error, not user error
> +            raise ValueError("Unknown feature requested %s" % feature)

If we skip remove the crecord thingy, in my opinion we can drop this 
entirely.

interface('clownyfeature') would return the value in `ui.interface`. And 
possible value of ui.interface MUST be understood by the underlying feature.

> +        availablefeatureinterfaces = defaultfeaturesinterfaces[feature].keys()
> +        defaultinterface = defaultfeaturesinterfaces[feature][defaultinterface]
> +
> +        f = self.config("ui", "interface.%s" % feature, None)
> +        if f is None:
> +            return defaultinterface

Again, we could just use 'defaultinterface' as the fallback value.

> +        else:
> +            if f not in availablefeatureinterfaces:
> +                self.warn(_("invalid value for ui.interface.%s: %s\n")
> +                          % (feature, f))
> +                self.warn(_("(using default interface instead: %s)\n")
> +                          % defaultinterface)

ditto about double line warning.

> +                return defaultinterface
> +            else:
> +                return defaultfeaturesinterfaces[feature][f]
> +
>       def interactive(self):
>           '''is interactive input allowed?
Augie Fackler - Feb. 13, 2016, 11:37 p.m.
> On Feb 12, 2016, at 10:40 AM, Pierre-Yves David <pierre-yves.david@ens-lyon.org> wrote:
> 
> 
> 
> On 02/11/2016 05:14 PM, Laurent Charignon wrote:
>> # HG changeset patch
>> # User Laurent Charignon <lcharignon@fb.com>
>> # Date 1455210657 28800
>> #      Thu Feb 11 09:10:57 2016 -0800
>> # Node ID 53494a3e515b7744759dfa064685b1b8299fffb7
>> # Parent  01a5143cd25f285f8c745a92986cd7186bb32c90
>> ui: add new config flag for interface selection
>> 
>> This patch introduces a new config flag ui.interface to select the interface
>> for interactive commands. It currently only applies to chunks selection.
>> The config can be overridden on a per feature basis with the flag
>> ui.interface.<feature>.
>> 
>> features for the moment can only be 'chunkselector', moving forward we expect
>> to have 'histedit' and other commands there.
>> 
>> If an incorrect value is given to ui.interface we print a warning and use the
>> default interface: text. If HGPLAIN is specified we also use the default
>> interface: text.
> 
> I think we should have a "auto" (name may varies) value, pick the currently best interface per sub feature.

I vehemently disagree with this idea, as I’ve stated before. I strongly feel that curses or other such horrors should be opt-in (see also the pain we’re experiencing with progress, color, and prompts being broken on Windows, if you want something other than opinion.)

(other than this point, I don’t disagree with pyd's review)

> For example we already have a "curses" ui in storage for histedit, but is definitly not polished enough yet. We'll want to use curses for chunk selection as a default long before we might want curses ui for histedit.
> 
> That would be the default. Don't update your patch for that, we can follow up on that if people agree.

> 
>> diff --git a/mercurial/help/config.txt b/mercurial/help/config.txt
>> --- a/mercurial/help/config.txt
>> +++ b/mercurial/help/config.txt
>> @@ -1587,6 +1587,17 @@
>>  ``interactive``
>>      Allow to prompt the user. (default: True)
>> 
>> +``interface``
>> +    Select the default interface for interactive features (default: text).
>> +    Possible values are 'text' and 'curses'.
>> +
>> +``interface.chunkselector``
>> +    Select the interface for chunkselection.
> 
> "override interactive interface choice for chunk selection"
> 
> I think we should also explain what is "chunk selection" here. Most people will not be able to link this with "hg commit -i".
> 
> Maybe "chunk selection" should be "patch edition" ? (very low confidence guess, juste hitting at the idea of having a different name).
> 
>> +    Possible values are 'text' and 'curses'.
>> +    This config overrides the interface specified by ui.interface.
>> +    If ui.interface and ui.interface.chunkselector aren't set, we use the text
>> +    interface.
> 
> I think the last sentence is implies in the default of ui.interface and can be dropped.
> 
>>  ``logtemplate``
>>      Template string for commands that print changesets.
>> 
>> diff --git a/mercurial/ui.py b/mercurial/ui.py
>> --- a/mercurial/ui.py
>> +++ b/mercurial/ui.py
>> @@ -699,6 +699,73 @@
>>              return False
>>          return util.isatty(fh)
>> 
>> +    def interface(self, feature):
>> +        '''what interface to use for interactive console features?
> 
> small nits: we usually use """ for docstring (not ''')
> 
>> +
>> +        The interface is controlled by the value of `ui.interface` but also by
>> +        the value of feature-specific configuration. For example:
>> +
>> +        ui.interface.histedit = text
>> +        ui.interface.chunkselector = curses
>> +
>> +        Here the features are "histedit" and "chunkselector".
>> +
>> +        The configuration above means that the default interfaces for commands
>> +        is curses, the interface for histedit is text and the interface for
>> +        selecting chunk is crecord (the best curses interface available).
>> +
>> +        Consider the following exemple:
>> +        ui.interface = curses
>> +        ui.interface.histedit = text
>> +
>> +        Then histedit will use the text interface and chunkselector will use
>> +        the default curses interface (crecord at the moment).
>> +        '''
>> +        if self.plain():
>> +            return "text"
>> +
>> +        alloweddefaultinterfaces = ("text", "curses")
>> +        defaultfeaturesinterfaces = {
>> +            "chunkselector": {
>> +                "text": "text",
>> +                "curses": "crecord"
> 
> Why do we need to introduce a mention of crecord here? Could we do all this with "curses" and introduce the notion of value's hierarchy later?
> 
> small nits: verylowvariablenamestarttohurtreadability.
> 
>> +            }
>> +        }
>> +
>> +        # Default interface for all the features
>> +        defaultinterface = "text"
>> +        i = self.config("ui", "interface", None)
>> +        if i is not None:
>> +            if i not in alloweddefaultinterfaces:
>> +                self.warn(_("invalid value for ui.interface: %s\n") % i)
>> +                self.warn(_("(using default interface instead %s)\n")
>> +                          % defaultinterface)
> 
> We rarely do double line message. Could we fit this on a single line?
> 
>> +            else:
>> +                defaultinterface = i
> 
> This is going to be quite confusing from a code flow perspective. You just move the value of a non-default interface into a variable called "defaultinterface"
> 
> I would use
> 
>  interface = self.config(…)
>  if invalide(interface):
>      warn()
>      interface = defaultinterface
> 
> instead.
> 
> You can also get rid of the 'None' testing by using 'defaultinterface' as the fallback value of self.config(…)
> 
> 
>> +        # Feature-specific interface
>> +        if feature not in defaultfeaturesinterfaces.keys():
>> +            # Programming error, not user error
>> +            raise ValueError("Unknown feature requested %s" % feature)
> 
> If we skip remove the crecord thingy, in my opinion we can drop this entirely.
> 
> interface('clownyfeature') would return the value in `ui.interface`. And possible value of ui.interface MUST be understood by the underlying feature.
> 
>> +        availablefeatureinterfaces = defaultfeaturesinterfaces[feature].keys()
>> +        defaultinterface = defaultfeaturesinterfaces[feature][defaultinterface]
>> +
>> +        f = self.config("ui", "interface.%s" % feature, None)
>> +        if f is None:
>> +            return defaultinterface
> 
> Again, we could just use 'defaultinterface' as the fallback value.
> 
>> +        else:
>> +            if f not in availablefeatureinterfaces:
>> +                self.warn(_("invalid value for ui.interface.%s: %s\n")
>> +                          % (feature, f))
>> +                self.warn(_("(using default interface instead: %s)\n")
>> +                          % defaultinterface)
> 
> ditto about double line warning.
> 
>> +                return defaultinterface
>> +            else:
>> +                return defaultfeaturesinterfaces[feature][f]
>> +
>>      def interactive(self):
>>          '''is interactive input allowed?
> 
> 
> --
> Pierre-Yves David
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

Patch

diff --git a/mercurial/help/config.txt b/mercurial/help/config.txt
--- a/mercurial/help/config.txt
+++ b/mercurial/help/config.txt
@@ -1587,6 +1587,17 @@ 
 ``interactive``
     Allow to prompt the user. (default: True)
 
+``interface``
+    Select the default interface for interactive features (default: text).
+    Possible values are 'text' and 'curses'.
+
+``interface.chunkselector``
+    Select the interface for chunkselection.
+    Possible values are 'text' and 'curses'.
+    This config overrides the interface specified by ui.interface.
+    If ui.interface and ui.interface.chunkselector aren't set, we use the text
+    interface.
+
 ``logtemplate``
     Template string for commands that print changesets.
 
diff --git a/mercurial/ui.py b/mercurial/ui.py
--- a/mercurial/ui.py
+++ b/mercurial/ui.py
@@ -699,6 +699,73 @@ 
             return False
         return util.isatty(fh)
 
+    def interface(self, feature):
+        '''what interface to use for interactive console features?
+
+        The interface is controlled by the value of `ui.interface` but also by
+        the value of feature-specific configuration. For example:
+
+        ui.interface.histedit = text
+        ui.interface.chunkselector = curses
+
+        Here the features are "histedit" and "chunkselector".
+
+        The configuration above means that the default interfaces for commands
+        is curses, the interface for histedit is text and the interface for
+        selecting chunk is crecord (the best curses interface available).
+
+        Consider the following exemple:
+        ui.interface = curses
+        ui.interface.histedit = text
+
+        Then histedit will use the text interface and chunkselector will use
+        the default curses interface (crecord at the moment).
+        '''
+        if self.plain():
+            return "text"
+
+        alloweddefaultinterfaces = ("text", "curses")
+        defaultfeaturesinterfaces = {
+            "chunkselector": {
+                "text": "text",
+                "curses": "crecord"
+            }
+        }
+
+        # Default interface for all the features
+        defaultinterface = "text"
+        i = self.config("ui", "interface", None)
+        if i is not None:
+            if i not in alloweddefaultinterfaces:
+                self.warn(_("invalid value for ui.interface: %s\n") % i)
+                self.warn(_("(using default interface instead %s)\n")
+                          % defaultinterface)
+            else:
+                defaultinterface = i
+
+
+        # Feature-specific interface
+        if feature not in defaultfeaturesinterfaces.keys():
+            # Programming error, not user error
+            raise ValueError("Unknown feature requested %s" % feature)
+
+
+        availablefeatureinterfaces = defaultfeaturesinterfaces[feature].keys()
+        defaultinterface = defaultfeaturesinterfaces[feature][defaultinterface]
+
+        f = self.config("ui", "interface.%s" % feature, None)
+        if f is None:
+            return defaultinterface
+        else:
+            if f not in availablefeatureinterfaces:
+                self.warn(_("invalid value for ui.interface.%s: %s\n")
+                          % (feature, f))
+                self.warn(_("(using default interface instead: %s)\n")
+                          % defaultinterface)
+                return defaultinterface
+            else:
+                return defaultfeaturesinterfaces[feature][f]
+
     def interactive(self):
         '''is interactive input allowed?
 
diff --git a/tests/test-commit-interactive-curses.t b/tests/test-commit-interactive-curses.t
--- a/tests/test-commit-interactive-curses.t
+++ b/tests/test-commit-interactive-curses.t
@@ -223,3 +223,58 @@ 
   hello world
 
 
+Check ui.interface logic for the chunkselector
+
+The default interface is text
+  $ chunkselectorinterface() {
+  > python <<EOF
+  > from mercurial import hg, ui, parsers;\
+  > repo = hg.repository(ui.ui(), ".");\
+  > print repo.ui.interface("chunkselector")
+  > EOF
+  > }
+  $ chunkselectorinterface
+  text
+
+The default curses interface is crecord
+  $ cat <<EOF >> $HGRCPATH
+  > [ui]
+  > interface = curses
+  > EOF
+  $ chunkselectorinterface
+  crecord
+
+It is possible to override the default interface with a feature specific
+interface
+  $ cat <<EOF >> $HGRCPATH
+  > [ui]
+  > interface = text
+  > interface.chunkselector = curses
+  > EOF
+
+  $ chunkselectorinterface
+  crecord
+
+If a bad interface name is given, we use the default value:
+  $ cat <<EOF >> $HGRCPATH
+  > [ui]
+  > interface = curses
+  > interface.chunkselector = blah
+  > EOF
+
+  $ chunkselectorinterface
+  invalid value for ui.interface.chunkselector: blah
+  (using default interface instead: crecord)
+  crecord
+  $ cat <<EOF >> $HGRCPATH
+  > [ui]
+  > interface = blah
+  > interface.chunkselector = blah
+  > EOF
+
+  $ chunkselectorinterface
+  invalid value for ui.interface: blah
+  (using default interface instead text)
+  invalid value for ui.interface.chunkselector: blah
+  (using default interface instead: text)
+  text