Thank you perl critic

[moregan]

Many small changes in response to Perl::Critic warnings.

[adamkennedy]

Beware the slavish use of critic, they are recommendations that can be
wrong in some cases.

Example, grep is often faster than any because it is built in. At least
until n gets pay a certain average threshold.

Check for a potential old commit that removed a previous any and replaced
with help.

And the file_content change seems a bit flippant...

Adam
On Mar 7, 2014 11:34 PM, "moregan" notifications@github.com wrote:

Many small changes in response to Perl::Critic warnings.

You can merge this Pull Request by running

git pull https://github.com/moregan/PPI thank-you-perl-critic

Or view, comment on, or merge it at:

#53
Commit Summary

• don't rely on error global after eval
• don't reuse variable names in scope
• more checking return of eval
• remove unneeded initializations
• dead code
• remove unused variables
• use regex with split
• trailing comma instead of semicolon
• fix punctuation and formatting
• use any instead of grep when all we're looking for is the first one
• Issue PPI::Token::Prototype::__TOKENIZER__on_char uses capture var in undefined state #50: illusion of use of $1 after failed match
• use three-arg open(), not two-arg
• use strict
• change foreach to map

File Changes

• M lib/PPI.pmhttps://github.com/Thank you perl critic #53/files#diff-0(7)
• M lib/PPI/Document.pmhttps://github.com/Thank you perl critic #53/files#diff-1(15)
• M lib/PPI/Element.pmhttps://github.com/Thank you perl critic #53/files#diff-2(10)
• M lib/PPI/Find.pmhttps://github.com/Thank you perl critic #53/files#diff-3(10)
• M lib/PPI/Lexer.pmhttps://github.com/Thank you perl critic #53/files#diff-4(7)
• M lib/PPI/Node.pmhttps://github.com/Thank you perl critic #53/files#diff-5(24)
• M lib/PPI/Normal.pmhttps://github.com/Thank you perl critic #53/files#diff-6(5)
• M lib/PPI/Statement/Include.pmhttps://github.com/Thank you perl critic #53/files#diff-7(2)
• M lib/PPI/Token/Attribute.pmhttps://github.com/Thank you perl critic #53/files#diff-8(2)
• M lib/PPI/Token/Pod.pmhttps://github.com/Thank you perl critic #53/files#diff-9(3)
• M lib/PPI/Token/Prototype.pmhttps://github.com/Thank you perl critic #53/files#diff-10(9)
• M lib/PPI/Token/Unknown.pmhttps://github.com/Thank you perl critic #53/files#diff-11(2)
• M lib/PPI/Token/Whitespace.pmhttps://github.com/Thank you perl critic #53/files#diff-12(2)
• M lib/PPI/Tokenizer.pmhttps://github.com/Thank you perl critic #53/files#diff-13(5)
• M lib/PPI/Transform.pmhttps://github.com/Thank you perl critic #53/files#diff-14(4)
• M t/04_element.thttps://github.com/Thank you perl critic #53/files#diff-15(7)
• M t/08_regression.thttps://github.com/Thank you perl critic #53/files#diff-16(5)
• M t/15_transform.thttps://github.com/Thank you perl critic #53/files#diff-17(39)
• M t/17_storable.thttps://github.com/Thank you perl critic #53/files#diff-18(8)
• M t/18_cache.thttps://github.com/Thank you perl critic #53/files#diff-19(4)
• M t/20_tokenizer_regression.thttps://github.com/Thank you perl critic #53/files#diff-20(11)
• M t/21_exhaustive.thttps://github.com/Thank you perl critic #53/files#diff-21(24)
• M t/lib/PPI.pmhttps://github.com/Thank you perl critic #53/files#diff-22(7)
• M t/ppi_token__quoteengine_full.thttps://github.com/Thank you perl critic #53/files#diff-23(1)
• M t/ppi_token_magic.thttps://github.com/Thank you perl critic #53/files#diff-24(2)
• M t/ppi_token_operator.thttps://github.com/Thank you perl critic #53/files#diff-25(6)
• M t/ppi_token_quotelike_words.thttps://github.com/Thank you perl critic #53/files#diff-26(1)
• M xt/author.thttps://github.com/Thank you perl critic #53/files#diff-27(3)
• M xt/meta.thttps://github.com/Thank you perl critic #53/files#diff-28(3)
• M xt/pmv.thttps://github.com/Thank you perl critic #53/files#diff-29(3)

Patch Links:

• https://github.com/adamkennedy/PPI/pull/53.patch
• https://github.com/adamkennedy/PPI/pull/53.diff

Reply to this email directly or view it on GitHubhttps://github.com//pull/53
.

[moregan]

Point taken on grep vs. any. In this context, any is taking 20% longer (on 5.16.3) than grep if they both process the same number of items. Since not processing the same number of items is an error condition for the method, you can it assume it almost never happens, so any doesn't help. That said, the any is more clear about intent, the grep on my machine takes on the order of 2 microseconds, and one could expect that merge() does not get used intensively, so I'd say it's a toss-up whether to revert the change.

$source was renamed because that name was already in use earlier in the same function.

[wchristian]

Went through them, and added those i agreed with, after some tidying. I think the only one i didn't agree with was the any, since it didn't actually speed things up.

Will be included in the next release.