| 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
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() ?
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.
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.
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.
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.
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.
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)