Bug #102545 for Perl-Critic-Pulp: RequireTrailingCommaAtNewline tweaks

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