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: 63472
Status: open
Priority: 0/
Queue: Perl-Critic

People
Owner: Nobody in particular
Requestors: EDAVIS [...] cpan.org
Cc:
AdminCc:

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



Subject: Suggested policy: comma instead of semicolon
Download (untitled) / with headers
text/plain 918b
I was surprised that perlcritic didn't catch the following mistake: print 'a', print 'b'; That first comma was intended to be a semicolon. Perl itself also gives no warning for this. I can think of a few possible policies that would catch it: - If a line ends with a comma, the next line should be indented. - Although the return code from the print() and say() builtins can be checked, it should not itself be printed directly. - print() and say() and perhaps other builtins should appear only as the left-hand part of an expression or statement. This would forbid stuff like $j = ($i++ && print 'hello') but (somehow) when print appears leftmost, as print('hello') || die then it would be allowed. What do you think? The indentation check is the most straightforward and I think would quickly catch bugs, but it may be a new departure for perlcritic to issue warnings based on code layout.
Download (untitled) / with headers
text/plain 3.1k
On Tue Nov 30 11:59:34 2010, EDAVIS wrote: Show quoted text
> I was surprised that perlcritic didn't catch the following mistake: > > print 'a', > print 'b'; > > That first comma was intended to be a semicolon. Perl itself also gives > no warning for this.
Hmmm. Tricky. I'll have to think about it. My first impression was that this was a bug in ValuesAndExpressions::ProhibitCommaSeparatedStatements. But there's actually nothing wrong with the general case of foo 1, 2, 3, bar 4, 5, 6; which is why Perl didn't complain. Your example looks strange to someone with a background in statement-oriented languages like Fortran, but in an expression-oriented language like Perl is another matter. Show quoted text
> > I can think of a few possible policies that would catch it: > > - If a line ends with a comma, the next line should be indented.
There are, in fact, Perl::Critic policies about code layout; specifically the seven CodeLayout:: policies. But these explicitly enforce layout policies. I suppose there could be something like CodeLayout::RequireArgumentListsToBeIndented, but the violation in this case would be something like "Indent argument list at line 2 column 4", which is an _extremely_ oblique way to say "you used a comma where you should have used a semicolon". And although I indent argument lists (and I presume you do too), the say-so of two people hardly makes this sufficiently a "best practice" to go in core Perl::Critic. Show quoted text
> > - Although the return code from the print() and say() builtins can be > checked, it should not itself be printed directly.
Devil's advocate: why not? But see below. Show quoted text
> > - print() and say() and perhaps other builtins should appear only as the > left-hand part of an expression or statement. > This would forbid stuff like $j = ($i++ && print 'hello') > but (somehow) when print appears leftmost, as print('hello') || die > then it would be allowed.
I'm also unsure about this approach. I have certainly written things like $i++ or print "Header\n"; which seems to me perfectly legit. Show quoted text
> > What do you think? The indentation check is the most straightforward > and I think would quickly catch bugs, but it may be a new departure for > perlcritic to issue warnings based on code layout.
See above for my take on the layout check. Another way to get at it might be a policy that prohibits passing the results of certain subroutines (be they built-ins or not) as arguments to other subroutines. The thing is, surely _someone_ has a legitimate reason to call foo 1, 2, print "Hello world!\n" I can't imagine what it is, but that doesn't count for much. If I personally were to write such a policy for core Perl::Critic, I would probably have it be disabled by default, by leaving the list of forbidden subroutine names empty. This whole thing would be a moot point if there were a thriving ecosystem of one-policy Perl-Critic-Something-Or-Other distributions from which people could pick and choose. If such a policy didn't already exist it could be written, people who agree with it could install it, and people who don't could ignore it. But that hasn't happened. Maybe one of the other maintainers has a better idea. Tom Wyant
Subject: Re: [rt.cpan.org #63472] Suggested policy: comma instead of semicolon
Date: Wed, 1 Dec 2010 00:20:55 -0800
To: bug-Perl-Critic [...] rt.cpan.org
From: Jeffrey Thalhammer <jeff [...] imaginative-software.com>
Download (untitled) / with headers
text/plain 1.2k
On Nov 30, 2010, at 8:59 AM, EDAVIS via RT wrote: Show quoted text
> - Although the return code from the print() and say() builtins can be > checked, it should not itself be printed directly.
I feel this rule is addressing a symptom of a problem, but not the actual problem. Show quoted text
> - print() and say() and perhaps other builtins should appear only as the > left-hand part of an expression or statement. > This would forbid stuff like $j = ($i++ && print 'hello') > but (somehow) when print appears leftmost, as print('hello') || die > then it would be allowed.
I think this may have some potential. Damian and Larry both advocate for having the *important* stuff on the left side of the page. Keep thinking about this. Show quoted text
> What do you think? The indentation check is the most straightforward > and I think would quickly catch bugs, but it may be a new departure for > perlcritic to issue warnings based on code layout.
Thus far, we've deferred all the layout issues to Perl::Tidy (via CodeLayout::RequireTidyCode). I'm not inclined to re-invent that wheel in the Perl::Critic core. But I have seen some folks write custom Policies to address particular formatting issues. Jeffrey Thalhammer Imaginative Software Systems vcard: http://www.imaginative-software.com/contact/jeff.vcf
Download (untitled) / with headers
text/plain 557b
It sounds like some kind of data flow analysis would work; the return value from print() can be used for most things but would give a warning if it is itself passed to print() in the same statement. Things like 'print(5, print(6))' are of course legal perl code, but unlikely to be what was intended, so in my opinion a suitable thing for a Perl::Critic policy to warn about. '$ret = print(6); print 5, $ret' would be fine. A policy for 'put the important thing on the left hand side' might be a good idea but I think it's a slightly different guideline.


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.