Patchwork [1,of,4] ui: add a function to read configuration related to devel warning

login
register
mail settings
Submitter Pierre-Yves David
Date March 11, 2015, 4:56 a.m.
Message ID <e9e42adc21a5c0a9bd68.1426049807@marginatus.alto.octopoid.net>
Download mbox | patch
Permalink /patch/7999/
State Changes Requested
Headers show

Comments

Pierre-Yves David - March 11, 2015, 4:56 a.m.
# HG changeset patch
# User Pierre-Yves David <pierre-yves.david@fb.com>
# Date 1426047011 25200
#      Tue Mar 10 21:10:11 2015 -0700
# Node ID e9e42adc21a5c0a9bd68079bdd209b7a086ad046
# Parent  4ef4e3c3c00693868ba428e21ac092e559e4fcea
ui: add a function to read configuration related to devel warning

We are about to introduce a class of configuration that will enable various
warnings targeted to developers. As we want both a wide and fine way to control
them, we introduce a special method to know if a warning of a given category
should be displayed or not.

First user will be introduced in the next changeset.
Yuya Nishihara - March 11, 2015, 2 p.m.
On Tue, 10 Mar 2015 21:56:47 -0700, Pierre-Yves David wrote:
> # HG changeset patch
> # User Pierre-Yves David <pierre-yves.david@fb.com>
> # Date 1426047011 25200
> #      Tue Mar 10 21:10:11 2015 -0700
> # Node ID e9e42adc21a5c0a9bd68079bdd209b7a086ad046
> # Parent  4ef4e3c3c00693868ba428e21ac092e559e4fcea
> ui: add a function to read configuration related to devel warning
> 
> We are about to introduce a class of configuration that will enable various
> warnings targeted to developers. As we want both a wide and fine way to control
> them, we introduce a special method to know if a warning of a given category
> should be displayed or not.
> 
> First user will be introduced in the next changeset.
> 
> diff --git a/mercurial/ui.py b/mercurial/ui.py
> --- a/mercurial/ui.py
> +++ b/mercurial/ui.py
> @@ -921,5 +921,11 @@ class ui(object):
>  
>          ui.write(s, 'label') is equivalent to
>          ui.write(ui.label(s, 'label')).
>          '''
>          return msg
> +
> +    def develflag(self, flag):
> +        """check if a devel warning is requested for flag X
> +
> +        We do not just "config" because we want to support a 'all' value"""
> +        return self.config('devel', 'all') or self.config('devel', flag)

should be configbool() ?
Pierre-Yves David - March 11, 2015, 10:17 p.m.
On 03/11/2015 07:00 AM, Yuya Nishihara wrote:
> On Tue, 10 Mar 2015 21:56:47 -0700, Pierre-Yves David wrote:
>> # HG changeset patch
>> # User Pierre-Yves David <pierre-yves.david@fb.com>
>> # Date 1426047011 25200
>> #      Tue Mar 10 21:10:11 2015 -0700
>> # Node ID e9e42adc21a5c0a9bd68079bdd209b7a086ad046
>> # Parent  4ef4e3c3c00693868ba428e21ac092e559e4fcea
>> ui: add a function to read configuration related to devel warning
>>
>> We are about to introduce a class of configuration that will enable various
>> warnings targeted to developers. As we want both a wide and fine way to control
>> them, we introduce a special method to know if a warning of a given category
>> should be displayed or not.
>>
>> First user will be introduced in the next changeset.
>>
>> diff --git a/mercurial/ui.py b/mercurial/ui.py
>> --- a/mercurial/ui.py
>> +++ b/mercurial/ui.py
>> @@ -921,5 +921,11 @@ class ui(object):
>>
>>           ui.write(s, 'label') is equivalent to
>>           ui.write(ui.label(s, 'label')).
>>           '''
>>           return msg
>> +
>> +    def develflag(self, flag):
>> +        """check if a devel warning is requested for flag X
>> +
>> +        We do not just "config" because we want to support a 'all' value"""
>> +        return self.config('devel', 'all') or self.config('devel', flag)
>
> should be configbool() ?

We are doing some magic with the 'all' value too. so this cannot just be 
configbool. we can build a generic function to handle "all" value if 
this becomes a general pattern.
Matt Mackall - March 11, 2015, 10:39 p.m.
On Wed, 2015-03-11 at 15:17 -0700, Pierre-Yves David wrote:
> 
> On 03/11/2015 07:00 AM, Yuya Nishihara wrote:
> > On Tue, 10 Mar 2015 21:56:47 -0700, Pierre-Yves David wrote:
> >> # HG changeset patch
> >> # User Pierre-Yves David <pierre-yves.david@fb.com>
> >> # Date 1426047011 25200
> >> #      Tue Mar 10 21:10:11 2015 -0700
> >> # Node ID e9e42adc21a5c0a9bd68079bdd209b7a086ad046
> >> # Parent  4ef4e3c3c00693868ba428e21ac092e559e4fcea
> >> ui: add a function to read configuration related to devel warning
> >>
> >> We are about to introduce a class of configuration that will enable various
> >> warnings targeted to developers. As we want both a wide and fine way to control
> >> them, we introduce a special method to know if a warning of a given category
> >> should be displayed or not.
> >>
> >> First user will be introduced in the next changeset.
> >>
> >> diff --git a/mercurial/ui.py b/mercurial/ui.py
> >> --- a/mercurial/ui.py
> >> +++ b/mercurial/ui.py
> >> @@ -921,5 +921,11 @@ class ui(object):
> >>
> >>           ui.write(s, 'label') is equivalent to
> >>           ui.write(ui.label(s, 'label')).
> >>           '''
> >>           return msg
> >> +
> >> +    def develflag(self, flag):
> >> +        """check if a devel warning is requested for flag X
> >> +
> >> +        We do not just "config" because we want to support a 'all' value"""
> >> +        return self.config('devel', 'all') or self.config('devel', flag)
> >
> > should be configbool() ?
> 
> We are doing some magic with the 'all' value too. so this cannot just be 
> configbool. we can build a generic function to handle "all" value if 
> this becomes a general pattern.

Still not clear. Why is it not:

  return self.configbool('devel', 'all') or self.configbool('devel',
flag)

I'm unable to spot any 'all' magic.
Pierre-Yves David - March 11, 2015, 11:34 p.m.
On 03/11/2015 03:39 PM, Matt Mackall wrote:
> On Wed, 2015-03-11 at 15:17 -0700, Pierre-Yves David wrote:
>>
>> On 03/11/2015 07:00 AM, Yuya Nishihara wrote:
>>> On Tue, 10 Mar 2015 21:56:47 -0700, Pierre-Yves David wrote:
>>>> # HG changeset patch
>>>> # User Pierre-Yves David <pierre-yves.david@fb.com>
>>>> # Date 1426047011 25200
>>>> #      Tue Mar 10 21:10:11 2015 -0700
>>>> # Node ID e9e42adc21a5c0a9bd68079bdd209b7a086ad046
>>>> # Parent  4ef4e3c3c00693868ba428e21ac092e559e4fcea
>>>> ui: add a function to read configuration related to devel warning
>>>>
>>>> We are about to introduce a class of configuration that will enable various
>>>> warnings targeted to developers. As we want both a wide and fine way to control
>>>> them, we introduce a special method to know if a warning of a given category
>>>> should be displayed or not.
>>>>
>>>> First user will be introduced in the next changeset.
>>>>
>>>> diff --git a/mercurial/ui.py b/mercurial/ui.py
>>>> --- a/mercurial/ui.py
>>>> +++ b/mercurial/ui.py
>>>> @@ -921,5 +921,11 @@ class ui(object):
>>>>
>>>>            ui.write(s, 'label') is equivalent to
>>>>            ui.write(ui.label(s, 'label')).
>>>>            '''
>>>>            return msg
>>>> +
>>>> +    def develflag(self, flag):
>>>> +        """check if a devel warning is requested for flag X
>>>> +
>>>> +        We do not just "config" because we want to support a 'all' value"""
>>>> +        return self.config('devel', 'all') or self.config('devel', flag)
>>>
>>> should be configbool() ?
>>
>> We are doing some magic with the 'all' value too. so this cannot just be
>> configbool. we can build a generic function to handle "all" value if
>> this becomes a general pattern.
>
> Still not clear. Why is it not:
>
>    return self.configbool('devel', 'all') or self.configbool('devel',
> flag)

1) Because if this is to be used in 24 different place, the odd for 
someone to forget the or statement is high.

But one can probably make a point that complication should probably be 
made when they are obviously a win instead of in advance.

> I'm unable to spot any 'all' magic.

2) I'm also expecting this function to grow some "all warnings but this 
blacklist" feature, but again, this probably fall into the "premature 
over-engineering bucket"

I'll drop this patch in the V2.
Pierre-Yves David - March 11, 2015, 11:52 p.m.
On 03/11/2015 04:34 PM, Pierre-Yves David wrote:
>
>
> On 03/11/2015 03:39 PM, Matt Mackall wrote:
>> On Wed, 2015-03-11 at 15:17 -0700, Pierre-Yves David wrote:
>>>
>>> On 03/11/2015 07:00 AM, Yuya Nishihara wrote:
>>>> On Tue, 10 Mar 2015 21:56:47 -0700, Pierre-Yves David wrote:
>>>>> # HG changeset patch
>>>>> # User Pierre-Yves David <pierre-yves.david@fb.com>
>>>>> # Date 1426047011 25200
>>>>> #      Tue Mar 10 21:10:11 2015 -0700
>>>>> # Node ID e9e42adc21a5c0a9bd68079bdd209b7a086ad046
>>>>> # Parent  4ef4e3c3c00693868ba428e21ac092e559e4fcea
>>>>> ui: add a function to read configuration related to devel warning
>>>>>
>>>>> We are about to introduce a class of configuration that will enable
>>>>> various
>>>>> warnings targeted to developers. As we want both a wide and fine
>>>>> way to control
>>>>> them, we introduce a special method to know if a warning of a given
>>>>> category
>>>>> should be displayed or not.
>>>>>
>>>>> First user will be introduced in the next changeset.
>>>>>
>>>>> diff --git a/mercurial/ui.py b/mercurial/ui.py
>>>>> --- a/mercurial/ui.py
>>>>> +++ b/mercurial/ui.py
>>>>> @@ -921,5 +921,11 @@ class ui(object):
>>>>>
>>>>>            ui.write(s, 'label') is equivalent to
>>>>>            ui.write(ui.label(s, 'label')).
>>>>>            '''
>>>>>            return msg
>>>>> +
>>>>> +    def develflag(self, flag):
>>>>> +        """check if a devel warning is requested for flag X
>>>>> +
>>>>> +        We do not just "config" because we want to support a 'all'
>>>>> value"""
>>>>> +        return self.config('devel', 'all') or self.config('devel',
>>>>> flag)
>>>>
>>>> should be configbool() ?
>>>
>>> We are doing some magic with the 'all' value too. so this cannot just be
>>> configbool. we can build a generic function to handle "all" value if
>>> this becomes a general pattern.
>>
>> Still not clear. Why is it not:
>>
>>    return self.configbool('devel', 'all') or self.configbool('devel',
>> flag)

Also, I cannot read, and totally missed the fact people have been 
pointing that I used `config` instead of `configbool`. I'm dropping the 
idea anyway.
Matt Mackall - March 12, 2015, midnight
On Wed, 2015-03-11 at 16:34 -0700, Pierre-Yves David wrote:
> 
> On 03/11/2015 03:39 PM, Matt Mackall wrote:
> > On Wed, 2015-03-11 at 15:17 -0700, Pierre-Yves David wrote:
> >>
> >> On 03/11/2015 07:00 AM, Yuya Nishihara wrote:
> >>> On Tue, 10 Mar 2015 21:56:47 -0700, Pierre-Yves David wrote:
> >>>> # HG changeset patch
> >>>> # User Pierre-Yves David <pierre-yves.david@fb.com>
> >>>> # Date 1426047011 25200
> >>>> #      Tue Mar 10 21:10:11 2015 -0700
> >>>> # Node ID e9e42adc21a5c0a9bd68079bdd209b7a086ad046
> >>>> # Parent  4ef4e3c3c00693868ba428e21ac092e559e4fcea
> >>>> ui: add a function to read configuration related to devel warning
> >>>>
> >>>> We are about to introduce a class of configuration that will enable various
> >>>> warnings targeted to developers. As we want both a wide and fine way to control
> >>>> them, we introduce a special method to know if a warning of a given category
> >>>> should be displayed or not.
> >>>>
> >>>> First user will be introduced in the next changeset.
> >>>>
> >>>> diff --git a/mercurial/ui.py b/mercurial/ui.py
> >>>> --- a/mercurial/ui.py
> >>>> +++ b/mercurial/ui.py
> >>>> @@ -921,5 +921,11 @@ class ui(object):
> >>>>
> >>>>            ui.write(s, 'label') is equivalent to
> >>>>            ui.write(ui.label(s, 'label')).
> >>>>            '''
> >>>>            return msg
> >>>> +
> >>>> +    def develflag(self, flag):
> >>>> +        """check if a devel warning is requested for flag X
> >>>> +
> >>>> +        We do not just "config" because we want to support a 'all' value"""
> >>>> +        return self.config('devel', 'all') or self.config('devel', flag)
> >>>
> >>> should be configbool() ?
> >>
> >> We are doing some magic with the 'all' value too. so this cannot just be
> >> configbool. we can build a generic function to handle "all" value if
> >> this becomes a general pattern.
> >
> > Still not clear. Why is it not:
> >
> >    return self.configbool('devel', 'all') or self.configbool('devel',
> > flag)
> 
> 1) Because if this is to be used in 24 different place, the odd for 
> someone to forget the or statement is high.

So confused. You understand we're trying to get you to change config to
configbool _in your helper function_, right? You are treating the
settings as booleans already.
Pierre-Yves David - March 12, 2015, 12:07 a.m.
On 03/11/2015 05:00 PM, Matt Mackall wrote:
> So confused. You understand we're trying to get you to change config to
> configbool _in your helper function_, right? You are treating the
> settings as booleans already.

I realized that while changing the implementation and realizing that it 
was not already using configbool. Do you like the helper function so 
much that I should reintroduce it ?

Patch

diff --git a/mercurial/ui.py b/mercurial/ui.py
--- a/mercurial/ui.py
+++ b/mercurial/ui.py
@@ -921,5 +921,11 @@  class ui(object):
 
         ui.write(s, 'label') is equivalent to
         ui.write(ui.label(s, 'label')).
         '''
         return msg
+
+    def develflag(self, flag):
+        """check if a devel warning is requested for flag X
+
+        We do not just "config" because we want to support a 'all' value"""
+        return self.config('devel', 'all') or self.config('devel', flag)