Bug #36125 for Perl-Critic: Suggest splitting ValuesAndExpressions::ProhibitInterpolationOfLiterals into two policies

Fri May 23 04:29:09 2008 EDAVIS [...] cpan.org - Ticket created

Subject:

Suggest splitting ValuesAndExpressions::ProhibitInterpolationOfLiterals into two policies

ValuesAndExpressions::ProhibitInterpolationOfLiterals is based on PBP page 51, 'Use interpolating string delimiters only for strings that actually interpolate.'. But that guideline is really in two parts: - don't double-quote a string when single-quoting it would have the same semantics, - if you need literal single quotes, write q{don't do that} instead of "don't do that". In my opinion the first of these is less controversial than the second. It would be good to split the policy in two, a weak rule which prohibits using "" for any string that doesn't have interpolation or literal ' characters, and a strong rule which prohibits using "" for any string that doesn't contain interpolation. I guess the weak rule could be promoted to severity 2 and the Damian-fundamentalist version stay in severity 1. If you agree, I'll write a patch.

Wed Jul 09 17:26:45 2008 thaljef [...] cpan.org - Correspondence added

On Fri May 23 04:29:09 2008, EDAVIS wrote: Show quoted text

> It would be good to split the policy in two, a weak rule which > prohibits using "" for any string that doesn't have interpolation or > literal ' characters, and a strong rule which prohibits using "" for any > string that doesn't contain interpolation.

I'd prefer to implement this as a configuration switch, rather than splitting up the policy. If the switch is set, then interpolated strings that contain ' but no other special characters would be exempt. By default, the switch would not be set. See http://search.cpan.org/~elliotjs/Perl-Critic-1.088/lib/Perl/Critic/DEVELOPER.pod for info on making Policies configurable. If you write the patch, I'll happily include it. -Jeff

Wed Jul 09 17:26:47 2008 The RT System itself - Status changed from 'new' to 'open'

Sun Jul 13 19:29:48 2008 EDAVIS [...] cpan.org - Correspondence added

Here's a patch as you suggest. I removed the bit of the doc saying that single-quoted strings are less work for the interpreter than double-quoted strings with the same content because they compile to the same bytecode: % perl -MO=Deparse -e '$x = "foo"' $x = 'foo'; -e syntax OK % perl -MO=Deparse -e "\$x = 'foo'" $x = 'foo'; -e syntax OK

Download Perl-Critic-1.088-patch
application/octet-stream 3.8k

Message body not shown because it is not plain text.

Sun Jul 13 20:35:48 2008 perl [...] galumph.com - Correspondence added

Subject:	Re: [rt.cpan.org #36125] Suggest splitting ValuesAndExpressions::ProhibitInterpolationOfLiterals into two policies
Date:	Sun, 13 Jul 2008 19:35:35 -0500
To:	bug-Perl-Critic [...] rt.cpan.org
From:	Elliot Shank <perl [...] galumph.com>

EDAVIS via RT wrote: Show quoted text

> Queue: Perl-Critic > Ticket <URL: http://rt.cpan.org/Ticket/Display.html?id=36125 > > > Here's a patch as you suggest. > > I removed the bit of the doc saying that single-quoted strings are less > work for the interpreter than double-quoted strings with the same > content because they compile to the same bytecode: > > % perl -MO=Deparse -e '$x = "foo"' > $x = 'foo'; > -e syntax OK > % perl -MO=Deparse -e "\$x = 'foo'" > $x = 'foo'; > -e syntax OK

And did you test compilation time?

Sun Jul 13 21:19:25 2008 perl [...] galumph.com - Correspondence added

Subject:	Re: [rt.cpan.org #36125] Suggest splitting ValuesAndExpressions::ProhibitInterpolationOfLiterals into two policies
Date:	Sun, 13 Jul 2008 20:19:12 -0500
To:	bug-Perl-Critic [...] rt.cpan.org
From:	Elliot Shank <perl [...] galumph.com>

Show quoted text

> EDAVIS via RT wrote:

>> Here's a patch as you suggest.

Applied, with changes. http://perlcritic.tigris.org/source/browse/perlcritic?rev=2586&view=rev

Sun Jul 13 21:26:26 2008 perl [...] galumph.com - Correspondence added

Subject:	Re: [rt.cpan.org #36125] Suggest splitting ValuesAndExpressions::ProhibitInterpolationOfLiterals into two policies
Date:	Sun, 13 Jul 2008 20:26:15 -0500
To:	bug-Perl-Critic [...] rt.cpan.org
From:	Elliot Shank <perl [...] galumph.com>

Changed name of option: http://perlcritic.tigris.org/source/browse/perlcritic?rev=2588&view=rev.

Sun Jul 13 22:16:12 2008 chris [...] chrisdolan.net - Correspondence added

Subject:	Re: [rt.cpan.org #36125] Suggest splitting ValuesAndExpressions::ProhibitInterpolationOfLiterals into two policies
Date:	Sun, 13 Jul 2008 21:15:53 -0500
To:	bug-Perl-Critic [...] rt.cpan.org
From:	Chris Dolan <chris [...] chrisdolan.net>

On Jul 13, 2008, at 7:35 PM, Elliot Shank via RT wrote: Show quoted text

> Queue: Perl-Critic > Ticket <URL: http://rt.cpan.org/Ticket/Display.html?id=36125 > > > EDAVIS via RT wrote:

>> Queue: Perl-Critic >> Ticket <URL: http://rt.cpan.org/Ticket/Display.html?id=36125 > >> >> Here's a patch as you suggest. >> >> I removed the bit of the doc saying that single-quoted strings are >> less >> work for the interpreter than double-quoted strings with the same >> content because they compile to the same bytecode: >>

> And did you test compilation time?

I just did (perl5.8.6) and, much to my surprise, the compile times were statistically identical. In retrospect that's probably because Perl still needs to scan single-quoted strings for special characters, just not as many of them. Chris

Sun Jul 13 22:17:37 2008 jeff [...] imaginative-software.com - Correspondence added

Subject:	Re: [rt.cpan.org #36125] Suggest splitting ValuesAndExpressions::ProhibitInterpolationOfLiterals into two policies
Date:	Sun, 13 Jul 2008 19:17:22 -0700
To:	bug-Perl-Critic [...] rt.cpan.org
From:	Jeffrey Thalhammer <jeff [...] imaginative-software.com>

Thanks for the patch, Ed. I appreciate all the help we can get! -Jeff

Mon Jul 14 11:49:10 2008 EDAVIS [...] cpan.org - Correspondence added

I think the test suite needs expanding to make sure the new code is covered: see attachment.

Index: t/ValuesAndExpressions/ProhibitInterpolationOfLiterals.run =================================================================== --- t/ValuesAndExpressions/ProhibitInterpolationOfLiterals.run (revision 2590) +++ t/ValuesAndExpressions/ProhibitInterpolationOfLiterals.run (working copy) @@ -7,7 +7,7 @@ #----------------------------------------------------------------------------- -## name Basic passing +## name Basic passing (0) ## failures 0 ## cut @@ -16,8 +16,18 @@ #----------------------------------------------------------------------------- -## name Code with all delimiters in configuration +## name Basic passing (1) ## failures 0 +## parms { allow_if_string_contains_single_quote => 1 } +## cut + +print 'this is literal'; +print q{this is literal}; + +#----------------------------------------------------------------------------- + +## name Code with all delimiters in configuration (0) +## failures 0 ## parms {allow => 'qq( qq{ qq[ qq/'} ## cut @@ -30,7 +40,21 @@ #----------------------------------------------------------------------------- -## name Code with not all delimiters in configuration +## name Code with all delimiters in configuration (1) +## failures 0 +## parms {allow => 'qq( qq{ qq[ qq/', allow_if_string_contains_single_quote => 1} +## cut + +$sql = qq(select foo from bar); +$sql = qq{select foo from bar}; +$sql = qq[select foo from bar]; +$sql = qq/select foo from bar/; + +is( pcritique($policy, \$code, \%config), 0, $policy); + +#----------------------------------------------------------------------------- + +## name Code with not all delimiters in configuration (0) ## failures 2 ## parms {allow => 'qq( qq{'} ## cut @@ -42,8 +66,20 @@ #----------------------------------------------------------------------------- -## name Configuration with only delimiters, no operators +## name Code with not all delimiters in configuration (1) ## failures 2 +## parms {allow => 'qq( qq{', allow_if_string_contains_single_quote => 1} +## cut + +$sql = qq(select foo from bar); +$sql = qq{select foo from bar}; +$sql = qq[select foo from bar]; +$sql = qq/select foo from bar/; + +#----------------------------------------------------------------------------- + +## name Configuration with only delimiters, no operators (0) +## failures 2 ## parms {allow => '() {}'} ## cut @@ -54,8 +90,20 @@ #----------------------------------------------------------------------------- -## name Configuration with matching closing delimiters +## name Configuration with only delimiters, no operators (1) ## failures 2 +## parms {allow => '() {}', allow_if_string_contains_single_quote => 1} +## cut + +$sql = qq(select foo from bar); +$sql = qq{select foo from bar}; +$sql = qq[select foo from bar]; +$sql = qq/select foo from bar/; + +#----------------------------------------------------------------------------- + +## name Configuration with matching closing delimiters (0) +## failures 2 ## parms {allow => 'qq() qq{}'} ## cut @@ -66,10 +114,22 @@ #----------------------------------------------------------------------------- -## name Disallow interpolationi f string contains single quote +## name Configuration with matching closing delimiters (1) ## failures 2 +## parms {allow => 'qq() qq{}', allow_if_string_contains_single_quote => 1} ## cut +$sql = qq(select foo from bar); +$sql = qq{select foo from bar}; +$sql = qq[select foo from bar]; +$sql = qq/select foo from bar/; + +#----------------------------------------------------------------------------- + +## name Disallow interpolation if string contains single quote +## failures 2 +## cut + $sql = "it's me"; $sql = "\'";

Tue Jul 22 11:16:24 2008 perl [...] galumph.com - Correspondence added

Subject:	Re: [rt.cpan.org #36125] Suggest splitting ValuesAndExpressions::ProhibitInterpolationOfLiterals into two policies
Date:	Tue, 22 Jul 2008 08:15:37 -0700
To:	bug-Perl-Critic [...] rt.cpan.org
From:	Elliot Shank <perl [...] galumph.com>

Jeffrey Thalhammer via RT wrote: Show quoted text

> Thanks for the patch, Ed. I appreciate all the help we can get!

Released in v1.090.

Sun Jul 27 19:38:20 2008 thaljef [...] cpan.org - Status changed from 'open' to 'resolved'

Bug #36125 for Perl-Critic: Suggest splitting ValuesAndExpressions::ProhibitInterpolationOfLiterals into two policies

Preferred bug tracker