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: 37886
Status: open
Priority: 0/
Queue: Perl-Critic
People
Owner: Nobody in particular
Requestors: EDAVIS [...] cpan.org
Cc:
AdminCc:
Bug Information
Severity: Unimportant
Broken in: 1.090
Fixed in: (no value)

History Show all quoted text
  Wed Jul 23 12:30:33 2008 EDAVIS [...] cpan.org - Ticket created
Subject: CodeLayout::ProhibitQuotedWordLists should be only for words
IMHO CodeLayout::ProhibitQuotedWordLists is a bit overzealous, banning for example my ($x, $y) = ('12-34', '56-78'); These two strings are not English words (or Perl \w+) and it's not really a list of words. To me, qw() is more for the cases when you have a variable-length list and they are words, not containing punctuation. So I would suggest some changes to soften this policy: - if the list is the RHS of an assignment to a tuple of variables, it's okay for the RHS to syntactically match the LHS. - only insist on qw() if the list is really a list of words and not odd things like '2008-07-23', '18:00', '+3' and so on.
  Wed Jul 23 12:47:09 2008 perl [...] galumph.com - Correspondence added
Subject: Re: [rt.cpan.org #37886] CodeLayout::ProhibitQuotedWordLists should be only for words
Date: Wed, 23 Jul 2008 09:46:56 -0700
To: bug-Perl-Critic [...] rt.cpan.org
From: Elliot Shank <perl [...] galumph.com>
EDAVIS via RT wrote: Show quoted text
> IMHO CodeLayout::ProhibitQuotedWordLists is a bit overzealous, banning > for example > > my ($x, $y) = ('12-34', '56-78'); > > These two strings are not English words (or Perl \w+) and it's not > really a list of words. To me, qw() is more for the cases when you have > a variable-length list and they are words, not containing punctuation.
Sorry, but I disagree. This policy should apply to anything that doesn't contain whitespace or interpolation.
  Wed Jul 23 12:47:10 2008 The RT System itself - Status changed from 'new' to 'open'
  Fri Jul 25 06:37:27 2008 EDAVIS [...] cpan.org - Correspondence added
Show quoted text
>Sorry, but I disagree. This policy should apply to anything that >doesn't contain whitespace or interpolation.
OK, but if there were a less strict version of the policy, it could be included at a higher severity level. Almost everyone would agree that qw() should be used for qw(Jack and Jill went up the hill) but something like my ($a, $b) = qw(1/1 +4); is more controversial, and might in some people's opinion be more clearly written as my ($a, $b) = ('1/1', '+4'); So the strictness might increase as you move from perlcritic -5 to -1.
  Fri Jul 25 08:15:38 2008 chris [...] chrisdolan.net - Correspondence added
Subject: Re: [rt.cpan.org #37886] CodeLayout::ProhibitQuotedWordLists should be only for words
Date: Fri, 25 Jul 2008 07:15:12 -0500
To: bug-Perl-Critic [...] rt.cpan.org
From: Chris Dolan <chris [...] chrisdolan.net>
On Jul 25, 2008, at 5:37 AM, EDAVIS via RT wrote: Show quoted text
> Queue: Perl-Critic > Ticket <URL: http://rt.cpan.org/Ticket/Display.html?id=37886 > >
>> Sorry, but I disagree. This policy should apply to anything that >> doesn't contain whitespace or interpolation.
> > OK, but if there were a less strict version of the policy, it could be > included at a higher severity level. Almost everyone would agree that > qw() should be used for > > qw(Jack and Jill went up the hill) > > but something like > > my ($a, $b) = qw(1/1 +4); > > is more controversial, and might in some people's opinion be more > clearly written as > > my ($a, $b) = ('1/1', '+4'); > > So the strictness might increase as you move from perlcritic -5 to -1.
I strongly side with EDAVIS on this one. I would never use qw() for the latter case. It's more confusing to the reader than straightforward single quotes. Chris
  Sat Jul 26 00:33:54 2008 jeff [...] imaginative-software.com - Correspondence added
Subject: Re: [rt.cpan.org #37886] CodeLayout::ProhibitQuotedWordLists should be only for words
Date: Fri, 25 Jul 2008 21:33:27 -0700
To: bug-Perl-Critic [...] rt.cpan.org
From: Jeffrey Thalhammer <jeff [...] imaginative-software.com>
On Jul 25, 2008, at 5:15 AM, Chris Dolan via RT wrote: Show quoted text
> > I strongly side with EDAVIS on this one. I would never use qw() for > the latter case. It's more confusing to the reader than > straightforward single quotes.
Ok, I can dig that. If we were to change the policy, how do you think we should handle something like this: @foo = ('eenie', '1.2' 'menie', '-14.7', 'minie', 'moe'); Some possibilities that came to mind as I wrote this: * qw() is never required if the list contains any non-words[1]. * qw() is not required if the ratio of non-words to words is less than X. * qw() is only required if there are more than X contiguous words. [1] I'm not exactly sure what constitutes a non-word. From the examples that have been given so far, it looks like a non-word is anything that contains only numbers and/or punctuation. Please clarify if you can. -Jeff
  Sat Jul 26 08:47:36 2008 EDAVIS [...] cpan.org - Correspondence added
Show quoted text
>* qw() is never required if the list contains any non-words[1]. >* qw() is not required if the ratio of non-words to words is less than X. >* qw() is only required if there are more than X contiguous words.
Since false positives suck, and there is no real bug addressed by this policy (it is purely a stylistic issue, unlike say the policies for eval {} or open() which prevent bugs), I suggest going with the simplest and most permissive choice, the first. I think a word matches [:alpha:]+ and anything which is not a word is a non-word.
  Sat Jul 26 21:15:47 2008 chris [...] chrisdolan.net - Correspondence added
Subject: Re: [rt.cpan.org #37886] CodeLayout::ProhibitQuotedWordLists should be only for words
Date: Sat, 26 Jul 2008 20:15:29 -0500
To: bug-Perl-Critic [...] rt.cpan.org
From: Chris Dolan <chris [...] chrisdolan.net>
On Jul 26, 2008, at 7:47 AM, EDAVIS via RT wrote: Show quoted text
> Queue: Perl-Critic > Ticket <URL: http://rt.cpan.org/Ticket/Display.html?id=37886 > >
>> * qw() is never required if the list contains any non-words[1]. >> * qw() is not required if the ratio of non-words to words is less >> than X. >> * qw() is only required if there are more than X contiguous words.
> > Since false positives suck, and there is no real bug addressed by this > policy (it is purely a stylistic issue, unlike say the policies for > eval > {} or open() which prevent bugs), I suggest going with the simplest > and > most permissive choice, the first. > > I think a word matches [:alpha:]+ and anything which is not a word > is a > non-word.
I second that suggestion. Chris
  Sat Jul 26 23:45:51 2008 jeff [...] imaginative-software.com - Correspondence added
Subject: Re: [rt.cpan.org #37886] CodeLayout::ProhibitQuotedWordLists should be only for words
Date: Sat, 26 Jul 2008 20:45:30 -0700
To: bug-Perl-Critic [...] rt.cpan.org
From: Jeffrey Thalhammer <jeff [...] imaginative-software.com>
Completed in revision 2616. Have a nice day! -Jeff
  Tue Sep 02 18:39:17 2008 thaljef [...] cpan.org - Correspondence added
Fixed and released in version 1.092.
  Tue Sep 02 18:39:20 2008 thaljef [...] cpan.org - Status changed from 'open' to 'resolved'
  Wed Sep 03 08:10:52 2008 EDAVIS [...] cpan.org - Correspondence added
Could you have another look at the fix? Even with 1.092 the original example still gives an unexpected result. % cat test my ($x, $y) = ('12-34', '56-78'); % perlcritic --single-policy CodeLayout::ProhibitQuotedWordLists test List of quoted literal words at line 1, column 15. Use 'qw()' instead. (Severity: 2) % perlcritic --version 1.092 I had expected that because neither '12-34' nor '56-78' are words, using qw() for the list would no longer be required.
  Wed Sep 03 08:10:56 2008 The RT System itself - Status changed from 'resolved' to 'open'
  Wed Sep 03 12:09:19 2008 jeff [...] imaginative-software.com - Correspondence added
Subject: Re: [rt.cpan.org #37886] CodeLayout::ProhibitQuotedWordLists should be only for words
Date: Wed, 3 Sep 2008 09:08:49 -0700
To: bug-Perl-Critic [...] rt.cpan.org
From: Jeffrey Thalhammer <jeff [...] imaginative-software.com>
On Sep 3, 2008, at 5:10 AM, EDAVIS via RT wrote: Show quoted text
> Queue: Perl-Critic > Ticket <URL: http://rt.cpan.org/Ticket/Display.html?id=37886 > > > Could you have another look at the fix? Even with 1.092 the original > example still gives an unexpected result. > > I had expected that because neither '12-34' nor '56-78' are words, > using > qw() for the list would no longer be required.
It looks like we used m/[\w-]+/ as our definition of "word characters". So by that definition, the policy is behaving correctly. But you could certainly argue that our definition of a word is wrong. I'll check with Elliot on this. -Jeff
  Sat Jan 17 22:03:14 2009 perl [...] galumph.com - Correspondence added
Subject: Re: [rt.cpan.org #37886] CodeLayout::ProhibitQuotedWordLists should be only for words
Date: Sat, 17 Jan 2009 17:40:26 -0600
To: bug-Perl-Critic [...] rt.cpan.org
From: Elliot Shank <perl [...] galumph.com>
EDAVIS via RT wrote: Show quoted text
> I had expected that because neither '12-34' nor '56-78' are words, using > qw() for the list would no longer be required.
Sticking to nothing but word characters, what about my ($x, $y) = ('12_34', '56_78');
  Mon Jan 19 05:55:04 2009 EDAVIS [...] cpan.org - Correspondence added
Show quoted text
>Sticking to nothing but word characters, what about > > my ($x, $y) = ('12_34', '56_78');
I would say a 'word' is a string that: - matches /\A\w+\z/ - and does not match /\A[0-9_]+\z/ If every item in the list is a word then qw() should be used. If any of the items is not a word, or if the list is empty, then the policy should not complain. In the above example 12_34 is not a word because it matches /\A[0-9_]+\z/. So the policy just wouldn't have an opinion.
  Mon Jan 19 07:47:31 2009 chris [...] chrisdolan.net - Correspondence added
Subject: Re: [rt.cpan.org #37886] CodeLayout::ProhibitQuotedWordLists should be only for words
Date: Mon, 19 Jan 2009 06:46:37 -0600
To: bug-Perl-Critic [...] rt.cpan.org
From: Chris Dolan <chris [...] chrisdolan.net>
On Jan 19, 2009, at 4:55 AM, EDAVIS via RT wrote: Show quoted text
> - matches /\A\w+\z/ > - and does not match /\A[0-9_]+\z/
Just FYI: that can be encapsulated as /\A[:alpha:]\w*\z/ or equivalently /\A\p{IsAlpha}\w*\z/ Chris
  Wed Aug 19 12:43:29 2009 EDAVIS [...] cpan.org - Correspondence added
Would you like me to make a patch implementing the behaviour I proposed?
  Wed Aug 19 23:53:14 2009 perl [...] galumph.com - Correspondence added
Subject: Re: [rt.cpan.org #37886] CodeLayout::ProhibitQuotedWordLists should be only for words
Date: Wed, 19 Aug 2009 22:52:53 -0500
To: bug-Perl-Critic [...] rt.cpan.org
From: Elliot Shank <perl [...] galumph.com>
EDAVIS via RT wrote: Show quoted text
> Would you like me to make a patch implementing the behaviour I proposed?
Actually, by default, no. Going back to the example: Show quoted text
> my ($x, $y) = ('12_34', '56_78');
This is actually very different from my ($x, $y) = (12_34, 56_78); because underscores are only considered part of numbers in numeric literals. The underscore is a regular character like any other in a string. However, if you want to provide a patch that adds an option for a regex to match, I'll take it. Note that the regex needs to be compiled with the /xms options. The parameter parser (i.e. the value of "parser" in the parameter specification in supported_parameters()) that checks the value of the regex option should contain something like my $actual_regex; eval { $actual_regex = qr/$regex_string/xms; 1 } or throw_policy_value policy => $self->get_short_name(), option_name => $arguments{option_name}, option_value => $arguments{option_value}, message_suffix => qq<contains an invalid regular expression: "$regex_string">;