[perl #40482] [PATCH] Perl::Critic policy for perl -w, and unix-specific shebang lines

[Paul Cochrane]

# New Ticket Created by "Paul Cochrane"
# Please include the string: [perl #40482]
# in the subject line of all future correspondence about this issue.
# <URL: http://rt.perl.org/rt3/Ticket/Display.html?id=40482 >

Hi,

This patch adds a new policy for the Parrot Perl::Critic tests, namely
to check that the shebang line doesn't use 'perl -w', rather 'use
warnings;' and that the shebang line doesn't use something unix
specific such as '#!/usr/bin/perl' and rather '#! perl'. Would it be
a good idea to group all of the code standards-related stuff into a
directory CodeStandards? As such, should I then make a patch for
CodeLayout::UseParrotCoda to go under CodeStandards::UseParrotCoda
instead? I'm also wondering what the policy is on svn Id keywords in
files, and whether or not the svn:keywords property is set to a
particular value. I don't think there's a standard or anything
defined for this yet. Should there be? If so, should I put a request
into RT?

TIA

Regards,

Paul

files affected:

lib/Perl/Critic/Policy/CodeStandards/CheckPerlShebang.pm

[Will Coleda]

On Oct 9, 2006, at 5:51 AM, Paul Cochrane (via RT) wrote:

> # New Ticket Created by "Paul Cochrane"
> # Please include the string: [perl #40482]
> # in the subject line of all future correspondence about this issue.
> # <URL: http://rt.perl.org/rt3/Ticket/Display.html?id=40482 >
>
>
> Hi,
>
> This patch adds a new policy for the Parrot Perl::Critic tests, namely
> to check that the shebang line doesn't use 'perl -w', rather 'use
> warnings;' and that the shebang line doesn't use something unix
> specific such as '#!/usr/bin/perl' and rather '#! perl'. Would it be
> a good idea to group all of the code standards-related stuff into a
> directory CodeStandards? As such, should I then make a patch for
> CodeLayout::UseParrotCoda to go under CodeStandards::UseParrotCoda
> instead?

They're all coding standards. I say leave the Coda where it is, and
put this in the same grouping as 'use warnings' (i.e.
'TestingAndDebugging').

Also, instead of checking for "/usr/local" and then reporting a
violation, switch it. Check for the *allowed* version, and, failing
that, report a violation. Then you'll catch /sw/bin/perl, /opt/bin/
perl, C:\perl\bin\perl.exe, and others.

Modulo that, this is good, and should be applied. (And addresses the
other end of the "switch from -w to use warnings" ticket that Jerry
opened.)

> I'm also wondering what the policy is on svn Id keywords in
> files, and whether or not the svn:keywords property is set to a
> particular value. I don't think there's a standard or anything
> defined for this yet. Should there be? If so, should I put a request
> into RT?
>

This is already tested in t/distro/file_metadata.t.

> TIA
>
> Regards,
>
> Paul
>

<PATCH_SNIPPED>

--
Will "Coke" Coleda
wi...@coleda.com

[Chris Dolan]

Paul,

A few comments:
* No, this shouldn't go into UseParrotCoda. Separately enabled
policies are more flexible.
* In fact, yours should probably be broken into two policies. Perhaps
CodeStandards::ProhibitShebangWarningsArg
and
CodeStandards::RequirePortableShebang
* This would be a nice addition to core Perl::Critic!
* The -w catcher fails on "#!perl -T -w" or other variations on
argument lists. Perhaps forbid any arguments at all?
* The shebang line is always a PPI::Token::Comment and is always on
the first line. Take a look at this utility function from
Perl::Critic::Utils:

sub is_script {
my $doc = shift;
my $first_comment = $doc->find_first('PPI::Token::Comment');
return if !$first_comment;
return if $first_comment->location()->[0] != 1;
return $first_comment =~ m{ \A \#\! }mx;
}

Now that I'm talking about it, I should write a
Perl::Critic::Utils::get_shebang() function...

Chris

--
Chris Dolan, Software Developer, http://www.chrisdolan.net/
Public key: http://www.chrisdolan.net/public.key
vCard: http://www.chrisdolan.net/ChrisDolan.vcf

[Chris Dolan]

On Oct 9, 2006, at 11:50 AM, Paul Cochrane wrote:

> Will,

>
>> They're all coding standards. I say leave the Coda where it is, and
>> put this in the same grouping as 'use warnings' (i.e.
>> 'TestingAndDebugging').

> Ok, see attached patch.

This one is still a false negative on "#!perl -Tw" and is a false
positive on "package main; #!!! my co-worker provided this non-Perl-
licensed code to Parrot!!!". Yes, that's a highly contrived
example. :-) But the false positive would be avoidable by checking
the line and column number of the element.

Chris

--
Chris Dolan, Software Developer, Clotho Advanced Media Inc.
608-294-7900, fax 294-7025, 1435 E Main St, Madison WI 53703
vCard: http://www.chrisdolan.net/ChrisDolan.vcf

Clotho Advanced Media, Inc. - Creators of MediaLandscape Software
(http://www.media-landscape.com/) and partners in the revolutionary
Croquet project (http://www.opencroquet.org/)

[Paul Cochrane]

Chris,

> This one is still a false negative on "#!perl -Tw" and is a false positive

> on "package main; #!!! my co-worker provided this non-Perl-licensed code to

> Parrot!!!". Yes, that's a highly contrived example. :-) But the false
> positive would be avoidable by checking the line and column number of the
> element.

Well spotted! This is another good point to help me test my test;
woah is that meta...

Paul

[Paul Cochrane]

Chris,

Thanks for your comments, I'm still trying to hone my perl abilities
and I really appreciate your feedback. I certainly need help
sometimes with my regular expressions...

> A few comments:
> * No, this shouldn't go into UseParrotCoda. Separately enabled
> policies are more flexible.

Actually, I meant putting the UseParrotCoda under CodingStandards::,
but as Will pointed out the shebang stuff would probably be better
under TestingAndDebugging::.

> * In fact, yours should probably be broken into two policies. Perhaps
> CodeStandards::ProhibitShebangWarningsArg
> and
> CodeStandards::RequirePortableShebang

That's a very good idea, and I thought of that, but didn't know if it
was better to split things up than bunch together related tests.

> * This would be a nice addition to core Perl::Critic!

Do you want me to supply a patch for Perl::Critic too, or will the
file added to Parrot suffice?

> * The -w catcher fails on "#!perl -T -w" or other variations on
> argument lists. Perhaps forbid any arguments at all?

Hadn't thought of "perl -T", and I'll update the patch accordingly;
thanks :-). I also found that the -w catcher failed when I was testing
my patch to resubmit, so again, I think I need to improve my regexp
foo.

> * The shebang line is always a PPI::Token::Comment and is always on
> the first line.

Using PPI::Token::Comment will simplify the test a lot more. I'll
have a bit more of a play and see if I can get a better patch in.

Thanks again for your feedback!

Paul

[Chris Dolan]

On Oct 10, 2006, at 3:12 AM, Paul Cochrane wrote:

>> * This would be a nice addition to core Perl::Critic!
> Do you want me to supply a patch for Perl::Critic too, or will the
> file added to Parrot suffice?

That's completely up to you. You seem to have a knack for writing
policies, so we'd love to have the help with Perl::Critic. But
Parrot is a worthy cause too! :-) If you don't provide a P::C patch,
I'll probably do it myself eventually.

Chris

--

[Paul Cochrane]

Chris,

> This one is still a false negative on "#!perl -Tw" and is a false positive

> on "package main; #!!! my co-worker provided this non-Perl-licensed code to

> Parrot!!!". Yes, that's a highly contrived example. :-) But the false
> positive would be avoidable by checking the line and column number of the
> element.

I've been playing around for a while with the shebang line tests, and
it struck me that the is_script() function will return true on your
contrived example. Here's my reasoning: the line:

package main; #!!! my co-worker...

gets pulled apart (so-to-speak) by $doc->find_first(
'PPI::Token::Comment' ) and the string

#!!! my co-worker...

gets returned as the first comment, and this matches the regular
expression. m{\A \#!}mx Am I right? I thought it was an interesting,
albeit contrived example.

Also, the fact that you mention that the shebang should be the first
line lead me to think that maybe we could look for misplaced shebang
lines, and report an error there as well. I've got a patch for that
and can send this in too if you want.

Regards,

Paul

[Chris Dolan]

On Oct 10, 2006, at 11:03 AM, Paul Cochrane wrote:

> I've been playing around for a while with the shebang line tests, and
> it struck me that the is_script() function will return true on your
> contrived example.

Yeah, I saw that and fixed it in SVN yesterday. :-) It now checks
that column == 1 too.

> Also, the fact that you mention that the shebang should be the first
> line lead me to think that maybe we could look for misplaced shebang
> lines, and report an error there as well. I've got a patch for that
> and can send this in too if you want.

Interesting idea. You'll have to ensure it's a PPI::Token::Comment
and not a PPI::Token::Quote, for example.

Perhaps further discussion should move to the perlcritic.tigris.org
dev mailing list or to
http://rt.cpan.org/Dist/Display.html?Queue=perl-critic

[Paul Cochrane via RT]

Patches applied as of revisions r15848, r15860, and r15871.

Thanks,

Paul