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

[perlcritic] fix some severity 4 issues #1051

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions .perlcriticrc
Original file line number Diff line number Diff line change
Expand Up @@ -44,3 +44,12 @@ severity = 5

[Variables::ProhibitUnusedVariables]
severity = 5

[Modules::ProhibitMultiplePackages]
severity = 5

[Modules::RequireEndWithOne]
severity = 5

[ValuesAndExpressions::ProhibitConstantPragma]
severity = 5
2 changes: 1 addition & 1 deletion lib/Munin/Common/Defaults.pm.PL
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ print $fh <<'EOF';
sub get_defaults {
my ($class) = @_;

## no critic
## no critic qw(TestingAndDebugging::ProhibitNoStrict)

no strict 'refs';
my $defaults = {};
Expand Down
1 change: 1 addition & 0 deletions lib/Munin/Master/Node.pm
Original file line number Diff line number Diff line change
Expand Up @@ -793,6 +793,7 @@ sub _merge_into_str_no_dup
}

# Defines the URL::scheme for munin
## no critic qw(Modules::ProhibitMultiplePackages)
package URI::munin;

# We are like a generic server
Expand Down
13 changes: 7 additions & 6 deletions lib/Munin/Node/SpoolWriter.pm
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,11 @@ use Munin::Common::Utils qw( is_valid_hostname );

use Params::Validate qw( :all );
use List::Util qw( first );
use Readonly;
Copy link
Member

Choose a reason for hiding this comment

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

Not sure I am a huge fan of that one as constant is a core feature.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Far from knowing anything: Readonly seems to be a core library and const is a language feature, correct?

Copy link
Member

@steveschnepp steveschnepp Aug 25, 2018

Choose a reason for hiding this comment

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

Core library might be a little stretch. It's from CPAN.

Const is from a core lib.

Nothing is a language feature in term of const-ness.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Core library might be a little stretch. It's from CPAN.

uh - OK - my misunderstanding of a local path.

In this case I would also tend to stick to the core feature set.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@cgzones: can we stick to const or is there a good reason for Readonly?


use constant DEFAULT_TIME => 86_400; # put 1 day of results into a spool file
use constant MAXIMUM_AGE => 7; # remove spool files more than a week old
use constant DEFAULT_HOSTNAME => 'munin.example.com';
Readonly my $DEFAULT_TIME => 86_400; # put 1 day of results into a spool file
Readonly my $MAXIMUM_AGE => 7; # remove spool files more than a week old
Readonly my $DEFAULT_HOSTNAME => 'munin.example.com';

sub _snap_to_epoch_boundary { my $self = shift; return $_[0] - ($_[0] % $self->{interval_size}) }

Expand Down Expand Up @@ -50,19 +51,19 @@ sub new {
$self->{interval_size} = first { defined($_) and $_ > 0 } (
$validated->{interval_size},
$self->{metadata}->{interval_size},
DEFAULT_TIME
$DEFAULT_TIME
);

$self->{interval_keep} = first { defined($_) and $_ > 0 } (
$validated->{interval_keep},
$self->{metadata}->{interval_keep},
MAXIMUM_AGE,
$MAXIMUM_AGE,
);

$self->{hostname} = first { defined($_) and is_valid_hostname($_) } (
$validated->{hostname},
$self->{metadata}->{hostname},
DEFAULT_HOSTNAME,
$DEFAULT_HOSTNAME,
);

return $self;
Expand Down
2 changes: 2 additions & 0 deletions lib/Munin/Plugin/Framework.pm
Original file line number Diff line number Diff line change
Expand Up @@ -177,3 +177,5 @@ sub run {
}
}
}

1;
2 changes: 2 additions & 0 deletions lib/Munin/Plugin/HTTP.pm
Original file line number Diff line number Diff line change
Expand Up @@ -111,3 +111,5 @@ sub get_basic_credentials {

return $isproxy ? () : ($ENV{'http_username'}, $ENV{'http_password'});
}

1;
2 changes: 0 additions & 2 deletions script/munin-httpd
Original file line number Diff line number Diff line change
Expand Up @@ -59,8 +59,6 @@ sub handle_request
}
}

package main;

$ENV{PATH} = '/usr/bin:/bin';
Copy link
Member

Choose a reason for hiding this comment

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

I prefer to revert to package main; to obviously indicate that the above code was in a package.
It could be moved to another file, or the end of this one, but it all feels not as sweet as currently.

Copy link
Collaborator

Choose a reason for hiding this comment

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

cgzones: what do you think?


# start the server on port 4948
Expand Down