Patchwork [2,of,5,V2] color: use valid_effect in configstyles

login
register
mail settings
Submitter Sean Farley
Date April 7, 2014, 9:17 p.m.
Message ID <0baffb232925978f2e7a.1396905421@laptop.local>
Download mbox | patch
Permalink /patch/4253/
State Superseded
Headers show

Comments

Sean Farley - April 7, 2014, 9:17 p.m.
# HG changeset patch
# User Sean Farley <sean.michael.farley@gmail.com>
# Date 1396902826 18000
#      Mon Apr 07 15:33:46 2014 -0500
# Node ID 0baffb232925978f2e7a70f6cb2ebd770cda17d0
# Parent  20427149f64472231b279bc17ade980cae402d8c
color: use valid_effect in configstyles
Durham Goode - April 7, 2014, 9:44 p.m.
On 4/7/14, 2:17 PM, Sean Farley wrote:
> # HG changeset patch
> # User Sean Farley <sean.michael.farley@gmail.com>
> # Date 1396902826 18000
> #      Mon Apr 07 15:33:46 2014 -0500
> # Node ID 0baffb232925978f2e7a70f6cb2ebd770cda17d0
> # Parent  20427149f64472231b279bc17ade980cae402d8c
> color: use valid_effect in configstyles
>
> diff --git a/hgext/color.py b/hgext/color.py
> --- a/hgext/color.py
> +++ b/hgext/color.py
> @@ -326,13 +326,11 @@ def configstyles(ui):
>               continue
>           cfgeffects = ui.configlist('color', status)
>           if cfgeffects:
>               good = []
>               for e in cfgeffects:
> -                if not _terminfo_params and e in _effects:
> -                    good.append(e)
> -                elif e in _terminfo_params or e[:-11] in _terminfo_params:
> +                if valideffect(e):
>                       good.append(e)
>                   else:
>                       ui.warn(_("ignoring unknown color/effect %r "
>                                 "(configured in color.%s)\n")
>                               % (e, status))
>
Why do this?  The commit message doesn't explain why this is desirable, 
and the later patches don't seem to actually use it.  I'd also do 
patches #1 and #2 as a single patch, since the changes together are a 
nice logical unit of change.
Sean Farley - April 7, 2014, 10 p.m.
Durham Goode <durham@fb.com> writes:

> On 4/7/14, 2:17 PM, Sean Farley wrote:
>> # HG changeset patch
>> # User Sean Farley <sean.michael.farley@gmail.com>
>> # Date 1396902826 18000
>> #      Mon Apr 07 15:33:46 2014 -0500
>> # Node ID 0baffb232925978f2e7a70f6cb2ebd770cda17d0
>> # Parent  20427149f64472231b279bc17ade980cae402d8c
>> color: use valid_effect in configstyles
>>
>> diff --git a/hgext/color.py b/hgext/color.py
>> --- a/hgext/color.py
>> +++ b/hgext/color.py
>> @@ -326,13 +326,11 @@ def configstyles(ui):
>>               continue
>>           cfgeffects = ui.configlist('color', status)
>>           if cfgeffects:
>>               good = []
>>               for e in cfgeffects:
>> -                if not _terminfo_params and e in _effects:
>> -                    good.append(e)
>> -                elif e in _terminfo_params or e[:-11] in _terminfo_params:
>> +                if valideffect(e):
>>                       good.append(e)
>>                   else:
>>                       ui.warn(_("ignoring unknown color/effect %r "
>>                                 "(configured in color.%s)\n")
>>                               % (e, status))
>>
> Why do this?  The commit message doesn't explain why this is desirable, 
> and the later patches don't seem to actually use it.  I'd also do 
> patches #1 and #2 as a single patch, since the changes together are a 
> nice logical unit of change.

Sure, I could fold these patches together if that's what people
want. The reason this patch exists is to reduce code duplication and
have one place were 'is this a valid effect name' is defined.

I'll fold and update the patch description unless there are any objections.

Patch

diff --git a/hgext/color.py b/hgext/color.py
--- a/hgext/color.py
+++ b/hgext/color.py
@@ -326,13 +326,11 @@  def configstyles(ui):
             continue
         cfgeffects = ui.configlist('color', status)
         if cfgeffects:
             good = []
             for e in cfgeffects:
-                if not _terminfo_params and e in _effects:
-                    good.append(e)
-                elif e in _terminfo_params or e[:-11] in _terminfo_params:
+                if valideffect(e):
                     good.append(e)
                 else:
                     ui.warn(_("ignoring unknown color/effect %r "
                               "(configured in color.%s)\n")
                             % (e, status))