Skip Menu |
 

Preferred bug tracker

Please visit the preferred bug tracker to report your issue.

This queue is for tickets about the Perl-Critic CPAN distribution.

Report information
The Basics
Id: 65464
Status: open
Priority: 0/
Queue: Perl-Critic

People
Owner: Nobody in particular
Requestors: user42 [...] zip.com.au
Cc:
AdminCc:

Bug Information
Severity: Wishlist
Broken in: (no value)
Fixed in: (no value)

Attachments


Subject: ProhibitUnusualDelimiters relax when // and {} in pattern
Date: Sat, 05 Feb 2011 10:01:14 +1100
To: bug-Perl-Critic [...] rt.cpan.org
From: Kevin Ryde <user42 [...] zip.com.au>
Download (untitled) / with headers
text/plain 743b
As an idea for a feature, if ProhibitUnusualDelimiters sees that the regexp contains / and { then it could helpfully relax itself and not ask that you use one of those two chars -- on that basis that if they're in the pattern then they're not good choices as delimiters. I struck this in a slightly bizarre email addr thing # RFC2822 "atext" characters, with "-" last if ($str =~ m<[^[:alnum:][:space:]!#\$%&'*+/=?^_`{|}~-]>) { I chose m<> because /, { and } are all in the chars to match. I'm not sure how much ProhibitUnusualDelimiters might relax. Allowing any delims in this case would be simplest. Or if allow_all_brackets is on then demand one of those <>, [], (), provided probably they're not all in the pattern too ... etc!
Download (untitled) / with headers
text/plain 1.2k
On Fri Feb 04 18:01:37 2011, user42@zip.com.au wrote: Show quoted text
> As an idea for a feature, if ProhibitUnusualDelimiters sees that the > regexp contains / and { then it could helpfully relax itself and not ask > that you use one of those two chars -- on that basis that if they're in > the pattern then they're not good choices as delimiters. > > I struck this in a slightly bizarre email addr thing > > # RFC2822 "atext" characters, with "-" last > if ($str =~ m<[^[:alnum:][:space:]!#\$%&'*+/=?^_`{|}~-]>) { > > I chose m<> because /, { and } are all in the chars to match. > > I'm not sure how much ProhibitUnusualDelimiters might relax. Allowing > any delims in this case would be simplest. Or if allow_all_brackets is > on then demand one of those <>, [], (), provided probably they're not > all in the pattern too ... etc!
Thank you for the suggestion. I personally am not sanguine about allowing _all_ delimiters in this case, without an "I really meant to do this" (spelled "## no critic (ProhibitUnusualDelimiters)". On the other hand, Perl::Critic itself turns on allow_all_brackets when it checks itself. I can see behaving as though allow_all_brackets were on if the regexp contains both '/' and '{'. Perhaps some of the other developers have thoughts on this? Tom
Subject: Re: [rt.cpan.org #65464] ProhibitUnusualDelimiters relax when // and {} in pattern
Date: Sat, 30 Apr 2011 09:36:33 +1000
To: bug-Perl-Critic [...] rt.cpan.org
From: Kevin Ryde <user42 [...] zip.com.au>
Download (untitled) / with headers
text/plain 523b
"Tom Wyant via RT" <bug-Perl-Critic@rt.cpan.org> writes: Show quoted text
> > I can see behaving as > though allow_all_brackets were on if the regexp contains both '/' and > '{'.
I got as far as the few lines below. I think the concept is sound enough. Think of it either as don't (implicitly) recommend usual delimiters when they'd be worse than the unusual. Or conversely think of it as don't prohibit something which is actually fairly reasonable in the particular usage. Precisely when and by how much may have to worked through.
Index: lib/Perl/Critic/Policy/RegularExpressions/ProhibitUnusualDelimiters.pm =================================================================== --- lib/Perl/Critic/Policy/RegularExpressions/ProhibitUnusualDelimiters.pm (revision 4076) +++ lib/Perl/Critic/Policy/RegularExpressions/ProhibitUnusualDelimiters.pm (working copy) @@ -68,6 +68,18 @@ my ( $self, $elem, undef ) = @_; my $allowed_delimiters = $self->{_allowed_delimiters}; + + # If the regexp contains / and also either { or } then those "usual" + # delimiter choices are not very good. In this case automatically relax + # to allow_all_brackets, if that's not already enabled. + if (! $self->{_allow_all_brackets} ) { + my $body = $elem->get_match_string; + if ($body =~ m</> && $body =~ m<[{}]>) { + $allowed_delimiters = { %$allowed_delimiters, + map {$_=>1} @EXTRA_BRACKETS }; + } + } + foreach my $delimiter ($elem->get_delimiters()) { next if $allowed_delimiters->{$delimiter}; return $self->violation( $DESC, $EXPL, $elem ); @@ -106,14 +118,45 @@ s;foo;bar;; # worse s|\|\||\||; # eye-gouging bad +=head2 Regexps with / and {} +If the regexp contains C</> and also C<{> or C<}> then neither C<//> nor C<{}> +are particularly good choices for delimiters. In this case the policy +automatically relaxes to C<allow_all_brackets> described below. + + if ($str =~ m</foo/bar\{.*>) # ok, / and { in the pattern + +This automatic relaxing is experimental. It's intended not to implicitly +recommend C<//> or C<{}> when they'd be poor choices. Maybe it could go +further and if the C<allow_all_brackets> forms are poor too (any escaped, +commented or unbalanced occurrences of those brackets) then don't report +anything at all. + =head1 CONFIGURATION -There is one option for this policy, C<allow_all_brackets>. If this -is true, then, in addition to allowing C<//> and C<{}>, the other -matched pairs of C<()>, C<[]>, and C<< <> >> are allowed. +There is one option for this policy, C<allow_all_brackets>. +=over +=item C<allow_all_brackets> (boolean, default false) + +If true then allow matched pairs C<()>, C<[]>, and C<< <> >> as delimiters +too, in addition to C<//> and C<{}>. + + if ($sheep =~ m<baa*>) # ok, under allow_all_brackets + $str =~ s[some.*thing][repl]; + my $re = qr(some.*[xyz]-thing); + +=back + +For reference, when C<()> or C<[]> are the delimiters you can still use C<()> +groups or C<[]> or char classes. Likewise C<{}> repetition counts. Whether +doing so looks good in another matter, but this policy doesn't prohibit it. + + s/ab{2,5}cc/repl/; # ok, always + if ($fruit =~ m(ban(an)*a)) # ok, under allow_all_brackets + my $re = qr[ba[rz]-var]; # ok, under allow_all_brackets + =head1 CREDITS Initial development of this policy was supported by a grant from the Index: t/RegularExpressions/ProhibitUnusualDelimiters.run =================================================================== --- t/RegularExpressions/ProhibitUnusualDelimiters.run (revision 4076) +++ t/RegularExpressions/ProhibitUnusualDelimiters.run (working copy) @@ -16,6 +16,7 @@ qr/foo/; qr{foo}; +#----------------------------------------------------------------------------- ## name basic failures ## failures 25 ## cut @@ -78,7 +79,46 @@ s[foo]<>; s<foo><>; +# POD examples cut and paste + if ($sheep =~ m<baa*>) { } # ok, under allow_all_brackets + $str =~ s[some.*thing][repl]; + my $re = qr(some.*[xyz]-thing); + if ($fruit =~ m(ban(an)*a)) # ok, under allow_all_brackets + my $re = qr[ba[rz]-var]; # ok, under allow_all_brackets + + #----------------------------------------------------------------------------- +## name passes of "both / and { or }" automatic allow_all_brackets +## failures 0 +## cut + +# / and either { or } in the pattern, automatic allow_all_brackets +# so these are ok +m</foo/bar\{.*>; +m(/foo/bar\}.*); +m[foo/bar{2,5}]; + +# / and either { or } in the pattern, automatic allow_all_brackets +# POD examples cut and paste + if ($str =~ m</foo/bar\{.*>) {} # ok, / and { in the pattern + s/ab{2,5}cc/repl/; # ok, always + +# Kevin's rss2leafnode pattern which motivated this relaxation + # RFC2822 "atext" characters, with "-" last + if ($str =~ m<[^[:alnum:][:space:]!#\$%&'*+/=?^_`{|}~-]>) { } + +#----------------------------------------------------------------------------- +## name failures of not "both / and { or }" +## failures 3 +## cut + +# not both / and either { or } in the pattern, no automatic allow_all_brackets +# so these are bad +m<foobar\{.*>; +m(foobar\}.*); +m[foo/bar]; + +#----------------------------------------------------------------------------- # Local Variables: # mode: cperl # cperl-indent-level: 4


This service is sponsored and maintained by Best Practical Solutions and runs on Perl.org infrastructure.

Please report any issues with rt.cpan.org to rt-cpan-admin@bestpractical.com.