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)



Subject: RequireTrailingCommaAtNewline tweaks
Date: Fri, 6 Mar 2015 15:54:18 +0000
To: "'bug-Perl-Critic-Pulp [...] rt.cpan.org'" <bug-Perl-Critic-Pulp [...] rt.cpan.org>
From: Ed Avis <eda [...] waniasset.com>
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
______________________________________________________________________
Subject: Re: [rt.cpan.org #102545] RequireTrailingCommaAtNewline tweaks
Date: Sat, 07 Mar 2015 19:13:58 +1100
To: "Ed Avis via RT" <bug-Perl-Critic-Pulp [...] rt.cpan.org>
From: Kevin Ryde <user42_kevin [...] yahoo.com.au>
"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.
Subject: Re: [rt.cpan.org #102545] RequireTrailingCommaAtNewline tweaks
Date: Sat, 7 Mar 2015 16:17:59 +0000
To: "bug-Perl-Critic-Pulp [...] rt.cpan.org" <bug-Perl-Critic-Pulp [...] rt.cpan.org>
From: Ed Avis <eda [...] waniasset.com>
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.