[munin-doc]: fix taint mode and some cleanup #1060
Conversation
cgzones
commented
Aug 27, 2018
cgzones
commented
- fix several perlcritic warnings and
- format with perltidy
Pull Request Test Coverage Report for Build 2426
💛 - Coveralls |
cgzones
force-pushed
the
master_munin-doc
branch
from
e8d48b3
to
e763e51
Compare
There was a problem hiding this comment.
Sometimes it seems, that you tend to mix different changes into one pull request :)
(I share this urge sometimes, when there are too many obvious things to be improved)
Maybe next time you could split the topics of your changes into separate commits (git add -p is a wonderful tool for this) within one pull request. This also helps writing a precise commit message :)
Anyway: thank you for the proposal!
Please take a look at my comment regarding PATH. The rest are just nitpicks ...
script/munin-doc
Outdated
|
|
||
| # un-taint full path | ||
| $File::Find::name =~ /^(.*)$/x; | ||
| $_ eq $plugin |
There was a problem hiding this comment.
I am not used to the different local dialects of perl style, but ... && ... does not feel as readable as if (...) to me.
I may be wrong ...
There was a problem hiding this comment.
I did not touch the && but it maybe can be written as push( @found, $1 ) if $_ eq $plugin;
script/munin-doc
Outdated
|
|
||
| # -F Arguments are file names, not modules | ||
| push(@ARGV,'-F',@found); | ||
| push( @ARGV, '-F', $found_first ); |
There was a problem hiding this comment.
I am just wondering: is the use of an opening and closing space within brackets really a common style?
There was a problem hiding this comment.
It's a change from perltidy (probably http://perltidy.sourceforge.net/stylekey.html -> Define Horizontal Tightness).
I think it can be changed by -pt=2.
It's up to the two of you to decide :)
There was a problem hiding this comment.
@steveschnepp: what do you think?
(my python-infested taste would prefer to avoid whitespace here)
script/munin-doc
Outdated
| =head1 AUTHOR | ||
|
|
||
| Copyright (C) 2008-2009 Nicolai Langfeldt, Linpro AS | ||
|
|
||
| =head1 LICENSE | ||
| =head1 LICENSE AND COPYRIGHT |
There was a problem hiding this comment.
The overwhelming majority of files in master use LICENSE. Only very few use COPYRIGHT/License. or COPYRIGHT.
(see grep -hr "^=head1" | cut -c 8- | sort | uniq -c | sort -n)
Specifically here I think, the Copyright is specified in the AUTHOR section.
Or do you have a specific reason for this change?
There was a problem hiding this comment.
perlcritic suggested to use LICENSE AND COPYRIGHT. But probably one (the both of you) should set up a policy (maybe in Hacking.pod) what headers to use
There was a problem hiding this comment.
@steveschnepp: do you have an opinion on this matter?
If not, then I would tend to follow perlcritic's suggestion.
script/munin-doc
Outdated
|
|
||
| # -F Arguments are file names, not modules | ||
| push(@ARGV,'-F',@found); | ||
| push( @ARGV, '-F', $found_first ); |
There was a problem hiding this comment.
Just wondering: are opening and closing whitespace within brackets really common?
cgzones
force-pushed
the
master_munin-doc
branch
from
e763e51
to
59b9878
Compare
|
dropped the perltidy changes |
|
ping |
|
@steveschnepp: do you have an opinion on the remaining changes? |
|
@steveschnepp: the remaining changes look good to me. Do you have any comments? |
