Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[munin-doc]: fix taint mode and some cleanup #1060

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

cgzones
Copy link
Contributor

@cgzones cgzones commented Aug 27, 2018

  • fix several perlcritic warnings and
  • format with perltidy

@coveralls
Copy link

coveralls commented Aug 27, 2018

Pull Request Test Coverage Report for Build 2426

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 5 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.3%) to 62.424%

Files with Coverage Reduction New Missed Lines %
lib/Munin/Master/UpdateWorker.pm 5 77.37%
Totals Coverage Status
Change from base Build 2425: -0.3%
Covered Lines: 1020
Relevant Lines: 1634

💛 - Coveralls

Copy link
Collaborator

@sumpfralle sumpfralle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 );
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am just wondering: is the use of an opening and closing space within brackets really a common style?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@steveschnepp: what do you think?

(my python-infested taste would prefer to avoid whitespace here)

script/munin-doc Show resolved Hide resolved
script/munin-doc Show resolved Hide resolved
script/munin-doc Outdated
=head1 AUTHOR

Copyright (C) 2008-2009 Nicolai Langfeldt, Linpro AS

=head1 LICENSE
=head1 LICENSE AND COPYRIGHT
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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 );
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just wondering: are opening and closing whitespace within brackets really common?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see other comment

@cgzones cgzones mentioned this pull request Aug 27, 2018
@cgzones
Copy link
Contributor Author

cgzones commented Aug 29, 2018

dropped the perltidy changes

@cgzones
Copy link
Contributor Author

cgzones commented Sep 21, 2018

ping

@sumpfralle
Copy link
Collaborator

@steveschnepp: do you have an opinion on the remaining changes?

@sumpfralle
Copy link
Collaborator

@steveschnepp: the remaining changes look good to me. Do you have any comments?

@steveschnepp steveschnepp added this to the 2.999.13 milestone Jun 9, 2019
@sumpfralle sumpfralle modified the milestones: 2.999.15, 2.999.16 Oct 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

4 participants