Bug #43864 for ANSIColor: Validation of colors and modifiers

Wed Mar 04 23:20:09 2009 wyant [...] cpan.org - Ticket created

CC:	dev [...] perlcritic.tigris.org
Subject:	Validation of colors and modifiers

A validation mechanism for colors and modifiers would be helpful for those cases where they are specified at one time and used at a different time. Yes, you could always pass them to color() and trap the exception, but a cleaner way would be nice. Thank you for your time and attention, Tom Wyant

Thu Mar 05 01:22:08 2009 rra [...] stanford.edu - Correspondence added

Subject:	Re: [rt.cpan.org #43864] Validation of colors and modifiers
Date:	Wed, 04 Mar 2009 22:21:51 -0800
To:	bug-ANSIColor [...] rt.cpan.org
From:	Russ Allbery <rra [...] stanford.edu>

"Tom Wyant via RT" <bug-ANSIColor@rt.cpan.org> writes: Show quoted text

> A validation mechanism for colors and modifiers would be helpful for > those cases where they are specified at one time and used at a different > time. > > Yes, you could always pass them to color() and trap the exception, but a > cleaner way would be nice.

Like a method into which you could pass the sort of arguments that you would pass to color() and it would return true if they're known keywords and false if they're not? -- Russ Allbery (rra@stanford.edu) <http://www.eyrie.org/~eagle/>

Thu Mar 05 01:22:09 2009 The RT System itself - Status changed from 'new' to 'open'

Thu Mar 05 11:22:32 2009 wyant [...] cpan.org - Correspondence added

On Wed Mar 04 22:22:08 2009, rra@stanford.edu wrote: Show quoted text

> "Tom Wyant via RT" <bug-ANSIColor@rt.cpan.org> writes: >

> > A validation mechanism for colors and modifiers would be helpful for > > those cases where they are specified at one time and used at a different > > time. > > > > Yes, you could always pass them to color() and trap the exception, but a > > cleaner way would be nice.

> > Like a method into which you could pass the sort of arguments that you > would pass to color() and it would return true if they're known keywords > and false if they're not?

Thank you very much for your prompt response. That sounds good to me. I was planning to iterate through them, so I could say which (if any) was bogus. Something like: foreach (qw{bold fuschia}) { check_keyword($_) or die "Bogus keyword $_"; } I don't know whether this fits with your vision of how this should go. Another implementation would take a list and return any invalid keywords, or nothing if they are all valid: my @bogus; @bogus = check_keywords(qw{bold fuschia}) and die ("Bogus keyword(s): @bogus"); Obviously (I hope!) I'm not trying to name functions for you. My apologies for ticket 43865 -- must have pushed the wrong button somewhere. Hopefully RT is smart enough not to try to do this again. Tom Wyant

Sat Jul 04 22:26:26 2009 RRA [...] cpan.org - Correspondence added

On Thu Mar 05 11:22:32 2009, WYANT wrote: Show quoted text

> That sounds good to me. I was planning to iterate through them, so I > could say which (if any) was bogus. Something like: > > foreach (qw{bold fuschia}) { > check_keyword($_) or die "Bogus keyword $_"; > } > > I don't know whether this fits with your vision of how this should go.

I went to implement this, and then realized that the capability for doing this is already there and is almost as simple as you show above. So before adding a new function, I wanted to check with you and see if that would be sufficient. If so, I'm going to document it in the next release. color() throws an exception for an invalid attribute, so your code above is equivalent to: for (qw(bold fuschia)) { eval { color ($_) }; die "Bogus keyword $_" if $@; } Would that work for you, and do you think that's simple enough if it's documented in the man page?

Sat Jul 04 22:32:56 2009 RRA [...] cpan.org - Cc CRITICRE deleted

Sat Jul 04 23:24:37 2009 wyant [...] cpan.org - Correspondence added

On Sat Jul 04 22:26:26 2009, RRA wrote: Show quoted text

> On Thu Mar 05 11:22:32 2009, WYANT wrote:

> > That sounds good to me. I was planning to iterate through them, so I > > could say which (if any) was bogus. Something like: > > > > foreach (qw{bold fuschia}) { > > check_keyword($_) or die "Bogus keyword $_"; > > } > > > > I don't know whether this fits with your vision of how this should go.

> > I went to implement this, and then realized that the capability for > doing this is already there and is almost as simple as you show above. > So before adding a new function, I wanted to check with you and see if > that would be sufficient. If so, I'm going to document it in the next > release. > > color() throws an exception for an invalid attribute, so your code above > is equivalent to: > > for (qw(bold fuschia)) { > eval { color ($_) }; > die "Bogus keyword $_" if $@; > } > > Would that work for you, and do you think that's simple enough if it's > documented in the man page?

My problem is that I'm just the messenger. The request was made while wearing my "Perl::Critic maintenance programmer" hat, which is why they were on the cc: list. In that capacity I implemented a request to have Perl::Critic's critique coloring be configurable. I wanted a validation mechanism, realized there was nothing much documented in Term::ANSIColor 1.12, and was undisciplined enough to do my validation by checking %Term::ANSIColor::attributes. This of course broke when Term::ANSIColor 2.0 came out. One of the lead developers solved the problem I caused by copying the %ATTRIBUTES hash into the relevant place in Perl::Critic. I counterproposed using exceptions for color validation, but this was nixed because "exceptions should be exceptional." Hence this RT ticket. What I believe is wanted is a public method/static method/subroutine in Term::ANSIColor, something like sub check_keyword { return exists %ATTRIBUTES{pop @_}; } (with the name being of course entirely up to you). If you are willing to do this, fine. Otherwise, if you would be willing to document the exception mechanism as the "official" way to do this, I'll go back to the lead developers and see what happens. Thanks, Tom Wyant

Sat Jul 04 23:36:55 2009 rra [...] stanford.edu - Correspondence added

Subject:	Re: [rt.cpan.org #43864] Validation of colors and modifiers
Date:	Sat, 04 Jul 2009 20:36:34 -0700
To:	bug-ANSIColor [...] rt.cpan.org
From:	Russ Allbery <rra [...] stanford.edu>

"Tom Wyant via RT" <bug-ANSIColor@rt.cpan.org> writes: Show quoted text

> My problem is that I'm just the messenger. > > The request was made while wearing my "Perl::Critic maintenance > programmer" hat, which is why they were on the cc: list.

Yeah, sorry, I took them off because RT was totally not coping and was opening a new ticket every time anything was sent to this RT ticket because their mailing list was bouncing all the messages. Show quoted text

> One of the lead developers solved the problem I caused by copying the > %ATTRIBUTES hash into the relevant place in Perl::Critic. I > counterproposed using exceptions for color validation, but this was > nixed because "exceptions should be exceptional." Hence this RT > ticket. > > What I believe is wanted is a public method/static method/subroutine in > Term::ANSIColor, something like > > sub check_keyword { > return exists %ATTRIBUTES{pop @_}; > } > > (with the name being of course entirely up to you). > > If you are willing to do this, fine. Otherwise, if you would be > willing to document the exception mechanism as the "official" way to > do this, I'll go back to the lead developers and see what happens.

Well, I don't have any fundamental objections to such an interface, but I hate to introduce more interfaces than are necessary, which is why I was wondering if the existing exception checking would be enough. What I have right now in the documentation is: It's sometimes useful to check attributes for validity in one place, such as when reading a configuration file that specifies colors. To do that, run color on a possible attribute, catching any exceptions. If it throws an exception, the attribute name isn't valid. for my $attr (@attributes) { eval { color ($attr) }; warn "$attr is not valid\n" if $@; } If you could run that by them and see what they think, I'd appreciate it. If that doesn't work, I can go back and add an interface easily enough. Sorry about the long delay in turnaround on this. I've not had a lot of chance for module hacking lately. -- Russ Allbery (rra@stanford.edu) <http://www.eyrie.org/~eagle/>

Fri Jul 10 23:17:49 2009 wyant [...] cpan.org - Correspondence added

On Sat Jul 04 23:36:55 2009, rra@stanford.edu wrote: Show quoted text

> What I have right now in the documentation is: > > It's sometimes useful to check attributes for validity in one place, > such as when reading a configuration file that specifies colors. To > do that, run color on a possible attribute, catching any exceptions. > If it throws an exception, the attribute name isn't valid. > > for my $attr (@attributes) { > eval { color ($attr) }; > warn "$attr is not valid\n" if $@; > } > > If you could run that by them and see what they think, I'd appreciate > it. If that doesn't work, I can go back and add an interface easily > enough. > > Sorry about the long delay in turnaround on this. I've not had a lot of > chance for module hacking lately.

No problem (says the requester who took a week to reply). I dropped your proposed code onto the Perl::Critic dev list, and got back that the current workaround (equivalent to a cut-and-paste of the %ATTRIBUTES hash into Perl::Critic) was considered preferable to the solution you intend documenting. "Normal" exceptions are theoretically a problem in Perl 5, because of its lack of an exception stack, and I in fact shot my own foot off once trying to make something like this work. But it's a rare problem, and I'm not sure I could not have made it right by simply localizing $@. I suspect something like this is what is behind the resistance I'm getting. The above is an argument for some sort of is_valid() method, but I do not consider it a particularly strong argument, and I have purposely not tried to make it stronger. At this point, I look at things this way: Term::ANSIColor is your module, and you should put in it what you see fit; Perl::Critic is Messrs Shank and Thalhammer's and what should go in it is what they see fit. If it was my code, I'd go with something like foreach my $attr (@attributes) { local $@; eval { color ($attr); 1} or warn "$attr is not valid\n"; } , but it's not my code, and the lead programmers seem happy with what they have. So at this point, I think all I really want to do is to thank you for your time and trouble. Tom Wyant

Thu Sep 03 13:25:51 2009 wyant [...] cpan.org - Correspondence added

Thank you very much for colorvalid(). Code using it has just been committed to the Perl::Critic svn repository. Tom Wyant

Tue Dec 29 00:22:56 2009 RRA [...] cpan.org - Fixed in 2.02 added

Tue Dec 29 00:22:56 2009 RRA [...] cpan.org - Status changed from 'open' to 'resolved'

Bug #43864 for ANSIColor: Validation of colors and modifiers

Maintainer(s)' notes