Skip Menu |
 

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

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

People
Owner: Nobody in particular
Requestors: eda [...] waniasset.com
Cc:
AdminCc:

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



MIME-Version: 1.0
X-Spam-Status: No, score=-3.051 tagged_above=-99.9 required=10 tests=[AWL=1.150, BAYES_00=-1.9, RCVD_IN_DNSWL_MED=-2.3, SPF_HELO_PASS=-0.001] autolearn=ham
X-Spam-Flag: NO
Content-Language: en-US
content-type: text/plain; charset="utf-8"
Message-ID: <7E039918541B4C4183BFDB8F015C743032DEA82E [...] WCL-EXCH02.wcl.local>
X-Virus-Scanned: Debian amavisd-new at bestpractical.com
X-MS-Tnef-Correlator:
X-Env-Sender: eda [...] waniasset.com
X-Spam-Score: -3.051
Received: from localhost (localhost [127.0.0.1]) by hipster.bestpractical.com (Postfix) with ESMTP id 5D34F240213 for <cpan-bug+Perl-Critic-Pulp [...] hipster.bestpractical.com>; Fri, 6 Mar 2015 10:54:34 -0500 (EST)
Received: from hipster.bestpractical.com ([127.0.0.1]) by localhost (hipster.bestpractical.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id spPUY257W3St for <cpan-bug+Perl-Critic-Pulp [...] hipster.bestpractical.com>; Fri, 6 Mar 2015 10:54:32 -0500 (EST)
Received: from la.mx.develooper.com (x1.develooper.com [207.171.7.70]) by hipster.bestpractical.com (Postfix) with SMTP id 1646524020F for <bug-Perl-Critic-Pulp [...] rt.cpan.org>; Fri, 6 Mar 2015 10:54:31 -0500 (EST)
Received: (qmail 21855 invoked by alias); 6 Mar 2015 15:54:30 -0000
Received: from mail1.bemta14.messagelabs.com (HELO mail1.bemta14.messagelabs.com) (193.109.254.109) by la.mx.develooper.com (qpsmtpd/0.28) with ESMTP; Fri, 06 Mar 2015 07:54:27 -0800
Received: from [193.109.254.147] by server-5.bemta-14.messagelabs.com id 89/A4-03170-DADC9F45; Fri, 06 Mar 2015 15:54:21 +0000
Received: (qmail 17636 invoked from network); 6 Mar 2015 15:54:19 -0000
Received: from unknown (HELO WCL-EXCHCAS03.wcl.local) (213.212.127.155) by server-3.tower-27.messagelabs.com with AES128-SHA encrypted SMTP; 6 Mar 2015 15:54:19 -0000
Received: from WCL-EXCH02.wcl.local ([169.254.1.57]) by WCL-EXCHCAS03.wcl.local ([149.85.126.212]) with mapi id 14.03.0123.003; Fri, 6 Mar 2015 15:54:19 +0000
Delivered-To: cpan-bug+Perl-Critic-Pulp [...] hipster.bestpractical.com
Subject: RequireTrailingCommaAtNewline tweaks
X-MSG-Ref: server-3.tower-27.messagelabs.com!1425657259!15225544!1
Return-Path: <eda [...] waniasset.com>
Thread-Index: AdBYIyCLP3N1l4eaQrSvurdufYND6Q==
X-RT-Mail-Extension: perl-critic-pulp
X-Original-To: cpan-bug+Perl-Critic-Pulp [...] hipster.bestpractical.com
X-Spam-Check-BY: la.mx.develooper.com
X-Starscan-Received:
Date: Fri, 6 Mar 2015 15:54:18 +0000
X-Starscan-Version: 6.13.4; banners=waniasset.com,-,-
X-Spam-Level:
X-MS-Has-Attach:
Thread-Topic: RequireTrailingCommaAtNewline tweaks
X-Viruschecked: Checked
X-Originating-Ip: [213.212.127.155]
X-Originating-Ip: [149.85.127.178]
Accept-Language: en-US, en-GB
To: "'bug-Perl-Critic-Pulp [...] rt.cpan.org'" <bug-Perl-Critic-Pulp [...] rt.cpan.org>
Content-Transfer-Encoding: quoted-printable
From: Ed Avis <eda [...] waniasset.com>
X-RT-Original-Encoding: ascii
X-RT-Interface: Email
Content-Length: 3230
Download (untitled) / with headers
text/plain 3.1k
The intention of CodeLayout::RequireTrailingCommaAtNewline is a good one. Multi-line lists are easier to maintain if the last element has a trailing comma, just as a trailing semicolon on the last statement in a block makes code easier to write (Pascal with its requirement to omit the final semicolon was a PITA). But I suggest there are a few cases where it could be tweaked. One of these is when the list is just a single item. It may be that an expression is split over several lines, but that does not really imply the intention of having a list to which items can be added and removed. print CGI::p("something or other $blah: " . "$foo gives $foo_result, " . "$bar gives $bar_result" ); Here CGI::p takes only a single argument. The closing ) is on a new line because doing so makes it easier to add and remove parts of the string concatenated with the . operator. But putting a trailing comma would not make the code clearer; indeed it would become less clear, because the comma would imply that additional arguments could be added, when in fact only one argument is allowed. - Suggestion: the check should not fire for lists of just a single item. - I might even go further and say that when a list has two items, but they are a key => value pair separated by a fat comma, the check should not fire in this case either. But that is a nicety. Another case where a trailing comma may not be appropriate is in a list of here documents: my @a = ( <<END long text END , <<END more long text END ); An extra comma on a line itself after the last END would not make the code more readable. You may argue that the right way to do this is to put the commas earlier as <<END, but there may be reasons for the more cumbersome style. (Mine is that syntax highlighting in my editor requires it to work properly.) - Suggestion: if the last element of the list is a here document then the check should not fire. Lastly, the programmer may sometimes want to set some defaults and then override them with a hash, as foo({ raise_error => 1, sounds => 'quiet', ignore_case => 1, %options }); Here we have some defaults but %options overrides them since it appears after them in the list. %options is intended to stay the last item, and any new stuff added goes just before that line, not after it. Another difference is that of course %options is not really a list item or even a k=>v pair, but a whole list in itself. For these reasons I suggest a trailing comma may be less appropriate. - Suggestion: if the last 'item' of the list is not a scalar, but @something or %something, then don't require the trailing comma. Thank you for taking the time to read this bug report. Of the suggestions made I think that "don't fire for single-element lists" is the most important, and should be easy to implement. -- Ed Avis <eda@waniasset.com> Show quoted text
______________________________________________________________________ This email has been scanned by the Symantec Email Security.cloud service. For more information please visit http://www.symanteccloud.com
______________________________________________________________________
X-Ymail-Osg: Q3J23P8VM1lkai7QphMfTX1WyoIB4fA.7YLeQ6rovtyJDqd jVeJISBcG2tV117cAcmGDti8O89pnUkru5fth8WiARKJX6Ee7g4U29OMFOUq 02wzyAXBEvfJ.ssh_ukQyQt7BS7xhhCJ4MRsEXIDeZEG2MunEYB0Luw5XGTL O3qDmtL4iTU_6_HgQ.6xFlHv84WT5NtYDZPX7JYmvI6xkBnPEadA6OWNPaKJ 7CBpHl.ZcGDbBQhRITyW88ve.4uOY1ncjmmWJJ9QyiUFoHTz6yJ3zA6DbPdg kWYhRojPshO.BY47wDfiXvgrH9W_xPNDsQWLNqx54qHwYBrpuZUPq3roGB7D L0O..X3Mz1z6OOK9AdeIpY4l_JbmZI23TUsr6CVOs8l9u_1unXmY84FuXhNk DXo5QKTyZFz6PAFg6uObazw9b7nTKI.hPVSpMjNhdhBSHG.ZTEjlj0uPlJ2h sZEqNpyFxWsdfhk0PX6xGaEIve4DdIkFqe_LfZVY6Fkb7LKR9so8YVZHZouC VMmYC3w1SPE4ZLeKA6gqHjWJgAkq9KqUOgfHHKN6vUZP4bWxroixJFuunPqr b3RDnYsFNGs0cprQJSkm8w76uhg--
MIME-Version: 1.0
X-Spam-Status: No, score=-5.018 tagged_above=-99.9 required=10 tests=[AWL=0.981, BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, FREEMAIL_FROM=0.001, FROM_OUR_RT=-4, RCVD_IN_DNSWL_NONE=-0.0001] autolearn=ham
In-Reply-To: <rt-4.0.18-26070-1425657275-577.102545-4-0 [...] rt.cpan.org> (Ed Avis via's message of "Fri, 6 Mar 2015 10:54:35 -0500")
X-Spam-Flag: NO
X-Yahoo-Newman-ID: 192082.24802.bm [...] smtp216.mail.bf1.yahoo.com
X-RT-Interface: API
References: <RT-Ticket-102545 [...] rt.cpan.org> <7E039918541B4C4183BFDB8F015C743032DEA82E [...] WCL-EXCH02.wcl.local> <rt-4.0.18-26070-1425657275-577.102545-4-0 [...] rt.cpan.org>
X-Yahoo-SMTP: U8NKAo6swBAxDIMUovTitGGHebxdYQBZ5A--
X-Virus-Scanned: Debian amavisd-new at bestpractical.com
X-Yahoo-Newman-Property: ymail-3
Message-ID: <87ioed9qd5.fsf [...] blah.blah>
content-type: text/plain; charset="utf-8"
X-RT-Original-Encoding: utf-8
X-Spam-Score: -5.018
Authentication-Results: hipster.bestpractical.com (amavisd-new); dkim=pass header.i= [...] yahoo.com.au
Received: from localhost (localhost [127.0.0.1]) by hipster.bestpractical.com (Postfix) with ESMTP id 98635240359 for <cpan-bug+Perl-Critic-Pulp [...] hipster.bestpractical.com>; Sat, 7 Mar 2015 03:20:18 -0500 (EST)
Received: from hipster.bestpractical.com ([127.0.0.1]) by localhost (hipster.bestpractical.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id jqO6TP-TjGuT for <cpan-bug+Perl-Critic-Pulp [...] hipster.bestpractical.com>; Sat, 7 Mar 2015 03:20:17 -0500 (EST)
Received: from la.mx.develooper.com (x1.develooper.com [207.171.7.70]) by hipster.bestpractical.com (Postfix) with SMTP id EB88624020F for <bug-Perl-Critic-Pulp [...] rt.cpan.org>; Sat, 7 Mar 2015 03:20:16 -0500 (EST)
Received: (qmail 6919 invoked by alias); 7 Mar 2015 08:20:16 -0000
Received: from nm27-vm1.bullet.mail.bf1.yahoo.com (HELO nm27-vm1.bullet.mail.bf1.yahoo.com) (98.139.213.148) by la.mx.develooper.com (qpsmtpd/0.28) with ESMTP; Sat, 07 Mar 2015 00:20:13 -0800
Received: from [98.139.215.143] by nm27.bullet.mail.bf1.yahoo.com with NNFMP; 07 Mar 2015 08:20:06 -0000
Received: from [98.139.211.207] by tm14.bullet.mail.bf1.yahoo.com with NNFMP; 07 Mar 2015 08:20:06 -0000
Received: from [127.0.0.1] by smtp216.mail.bf1.yahoo.com with NNFMP; 07 Mar 2015 08:20:06 -0000
Delivered-To: cpan-bug+Perl-Critic-Pulp [...] hipster.bestpractical.com
Subject: Re: [rt.cpan.org #102545] RequireTrailingCommaAtNewline tweaks
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.4 (gnu/linux)
Return-Path: <user42_kevin [...] yahoo.com.au>
Dkim-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=yahoo.com.au; s=s2048; t=1425716406; bh=hb5bNifGmGRp5fTlNXzEVFAFvvf0BKhGeCWF0EVUT/4=; h=From:To:Subject:References:Date:In-Reply-To:From:Subject; b=stvp4CINKkxN3dZAbp+jQ7xZyZSefmFF1gcE8RpdPPUTwCFU/pFZUiBEq72vwmipHFYmgQOeH1Mv6DeK7BBOshB045bELXMYcPfCVVJrevYm0LgfeZ9UAdwr/x8j67cDINf0rvOi34+ARGFxKFu1OxDBN4edR204QS7AlJKkNs+NGyxSX2z3LcFCcghgUOmNKaEEqGpYvsg4VI1Y43qhhTo1XnbUpmHRxLugeunI8tVV03UAp0N3ndy90HQHnM06ykelKjgNS8GK21cHohP5AiuW9akL8ccLPAhB8HfNuYgeDsNsgKa5E3NPC2rCgEke0OfSgAE6AGjgAWFgJ4OU7A==
X-Spam-Check-BY: la.mx.develooper.com
X-Original-To: cpan-bug+Perl-Critic-Pulp [...] hipster.bestpractical.com
X-RT-Mail-Extension: perl-critic-pulp
Date: Sat, 07 Mar 2015 19:13:58 +1100
X-Spam-Level:
To: "Ed Avis via RT" <bug-Perl-Critic-Pulp [...] rt.cpan.org>
From: Kevin Ryde <user42_kevin [...] yahoo.com.au>
RT-Message-ID: <rt-4.0.18-22824-1425716419-202.102545-0-0 [...] rt.cpan.org>
Content-Length: 3165
"Ed Avis via RT" <bug-Perl-Critic-Pulp@rt.cpan.org> writes: Show quoted text
> > But I suggest there are a few cases where it could be tweaked. One of > these is when the list is just a single item. It may be that an > expression is split over several lines, but that does not really imply > the intention of having a list to which items can be added and > removed. > > print CGI::p("something or other $blah: " > . "$foo gives $foo_result, " > . "$bar gives $bar_result" > );
Are you inclined to exempt all one-item lists, or just one-item function calls? Maybe an "except_function_calls_one_arg" option for the latter. I'd think only function calls, since any @foo=() assignment can likely have more added to it, however its first element might be written. Show quoted text
> Here CGI::p takes only a single argument. The closing ) is on a new > line because doing so makes it easier to add and remove parts of the > string concatenated with the . operator. But putting a trailing comma > would not make the code clearer;
Yes. Though my rationale was that there's no way to tell if the function takes a fixed or variable number of arguments. If fixed then a final comma is unwanted, but how to tell that? Show quoted text
> - I might even go further and say that when a list has two items, but > they are a key => value pair separated by a fat comma, the check > should not fire in this case either. But that is a nicety.
Hmm. I don't think I agree with that bit. Show quoted text
> my @a = ( > <<END > long text > END > , > <<END > more long text > END > );
I exempted single-item here-documents from the from the first ticket, but not multiple. Show quoted text
> An extra comma on a line itself after the last END would not make the > code more readable. You may argue that the right way to do this is to > put the commas earlier as <<END,
That's what I think I would do, but whether it's more obscure by hiding the comma where you hardly see, that's another matter. :) Show quoted text
> - Suggestion: if the last element of the list is a here document then > the check should not fire.
I'm tempted to make an option "except_final_here" or some such name, intended for fans of inline heres, and have that option instead of the default single-item here exemption I put already. Show quoted text
> foo({ > raise_error => 1, > sounds => 'quiet', > ignore_case => 1, > %options > });
Hmm. I think a final comma there is my intention. The rationale would be that it's possible to add non-overridable bits after the %options. Of course nobody has to enable the policy if it's too terrible. :-) I started this one because I thought the "RequireTrailingCommas" asking for trailing commas absolutely everywhere was too much. Maybe others will think the same of mine. :-) Show quoted text
> - Suggestion: if the last 'item' of the list is not a scalar, but > @something or %something, then don't require the trailing comma.
I don't mind contemplating an option if you still like it. Bear in mind it's not always easy to identify a list-valued final expression. A plain @foo is ok, but spray some parens, do{}, map{}, or list-valued function call and it becomes harder.
MIME-Version: 1.0
X-Spam-Flag: NO
Content-ID: <C1FF5535EF3E8941A770E836C59069C8 [...] waniasset.com>
X-Virus-Scanned: Debian amavisd-new at bestpractical.com
content-type: text/plain; charset="utf-8"
X-RT-Original-Encoding: utf-8
X-Spam-Score: -5.626
Received: from localhost (localhost [127.0.0.1]) by hipster.bestpractical.com (Postfix) with ESMTP id 8610F2403E6 for <cpan-bug+Perl-Critic-Pulp [...] hipster.bestpractical.com>; Sat, 7 Mar 2015 11:18:12 -0500 (EST)
Received: from hipster.bestpractical.com ([127.0.0.1]) by localhost (hipster.bestpractical.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id FKBWMq6v3GMM for <cpan-bug+Perl-Critic-Pulp [...] hipster.bestpractical.com>; Sat, 7 Mar 2015 11:18:10 -0500 (EST)
Received: from la.mx.develooper.com (x1.develooper.com [207.171.7.70]) by hipster.bestpractical.com (Postfix) with SMTP id 054FF24020F for <bug-Perl-Critic-Pulp [...] rt.cpan.org>; Sat, 7 Mar 2015 11:18:09 -0500 (EST)
Received: (qmail 1846 invoked by alias); 7 Mar 2015 16:18:09 -0000
Received: from mail1.bemta14.messagelabs.com (HELO mail1.bemta14.messagelabs.com) (193.109.254.120) by la.mx.develooper.com (qpsmtpd/0.28) with ESMTP; Sat, 07 Mar 2015 08:18:06 -0800
Received: from [193.109.254.147] by server-16.bemta-14.messagelabs.com id BA/FD-02639-9B42BF45; Sat, 07 Mar 2015 16:18:01 +0000
Received: (qmail 16747 invoked from network); 7 Mar 2015 16:18:01 -0000
Received: from unknown (HELO WCL-EXCHCAS02.wcl.local) (80.169.169.167) by server-10.tower-27.messagelabs.com with AES128-SHA encrypted SMTP; 7 Mar 2015 16:18:01 -0000
Received: from WCL-EXCH02.wcl.local ([169.254.1.57]) by WCL-EXCHCAS02.wcl.local ([149.85.127.102]) with mapi id 14.03.0123.003; Sat, 7 Mar 2015 16:18:00 +0000
Delivered-To: cpan-bug+Perl-Critic-Pulp [...] hipster.bestpractical.com
X-MSG-Ref: server-10.tower-27.messagelabs.com!1425745081!15523414!1
Subject: Re: [rt.cpan.org #102545] RequireTrailingCommaAtNewline tweaks
X-Spam-Check-BY: la.mx.develooper.com
Thread-Index: AdBYIyCLP3N1l4eaQrSvurdufYND6QAjHF7IABCHiwA=
Date: Sat, 7 Mar 2015 16:17:59 +0000
X-Starscan-Version: 6.13.4; banners=waniasset.com,-,-
X-Spam-Level:
X-Viruschecked: Checked
To: "bug-Perl-Critic-Pulp [...] rt.cpan.org" <bug-Perl-Critic-Pulp [...] rt.cpan.org>
Content-Transfer-Encoding: quoted-printable
In-Reply-To: <rt-4.0.18-22824-1425716419-1021.102545-6-0 [...] rt.cpan.org>
X-Spam-Status: No, score=-5.626 tagged_above=-99.9 required=10 tests=[AWL=2.575, BAYES_00=-1.9, FROM_OUR_RT=-4, RCVD_IN_DNSWL_MED=-2.3, SPF_HELO_PASS=-0.001] autolearn=ham
X-RT-Interface: API
Content-Language: en-US
References: <RT-Ticket-102545 [...] rt.cpan.org> <7E039918541B4C4183BFDB8F015C743032DEA82E [...] WCL-EXCH02.wcl.local> <rt-4.0.18-26070-1425657275-577.102545-4-0 [...] rt.cpan.org> <87ioed9qd5.fsf [...] blah.blah> <rt-4.0.18-22824-1425716419-1021.102545-6-0 [...] rt.cpan.org>
Message-ID: <D1206679.47831%eda [...] waniasset.com>
X-MS-Tnef-Correlator:
X-Env-Sender: eda [...] waniasset.com
User-Agent: Microsoft-MacOutlook/14.4.8.150116
Return-Path: <eda [...] waniasset.com>
X-RT-Mail-Extension: perl-critic-pulp
X-Original-To: cpan-bug+Perl-Critic-Pulp [...] hipster.bestpractical.com
X-Starscan-Received:
X-MS-Has-Attach:
Thread-Topic: [rt.cpan.org #102545] RequireTrailingCommaAtNewline tweaks
X-Originating-Ip: [80.169.169.167]
X-Originating-Ip: [149.85.126.6]
Accept-Language: en-US, en-GB
From: Ed Avis <eda [...] waniasset.com>
RT-Message-ID: <rt-4.0.18-20753-1425745093-281.102545-0-0 [...] rt.cpan.org>
Content-Length: 1158
Download (untitled) / with headers
text/plain 1.1k
Show quoted text
>Are you inclined to exempt all one-item lists, or just one-item function >calls?
I don't have a strong view on that. Show quoted text
>Though my rationale was that there's no way to tell if the >function takes a fixed or variable number of arguments.
There isn't, but false positives are usually worse than false negatives, so better to assume that a single argument implies a one-argument function. FWIW, I usually find that variable-args functions are called passing an array, as foo($x, @stuff) rather than with a literal argument list. Show quoted text
>Bear in mind >it's not always easy to identify a list-valued final expression. >A plain @foo is ok, but spray some parens, do{}, map{}, or list-valued >function call and it becomes harder.
Yes, I chose my words carefully to cover only an @array or %hash as the last argument, not other stuff. But this tweak is less important than the others. Show quoted text
______________________________________________________________________ This email has been scanned by the Symantec Email Security.cloud service. For more information please visit http://www.symanteccloud.com
______________________________________________________________________


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.