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.

Sign up for GitHub

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

Jump to bottom

[perlcritic] fix some severity 4 issues #1051

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

[perlcritic] fix some severity 4 issues #1051

wants to merge 1 commit into from

Conversation

cgzones
Copy link
Contributor

@cgzones cgzones commented Aug 21, 2018

No description provided.

@cgzones
Copy link
Contributor Author

cgzones commented Aug 21, 2018

I looked over perlcritic severity 4 warnings and like to ask what to do about the following warnings:

fix or silence them?

@coveralls
Copy link

coveralls commented Aug 21, 2018 •
edited
Loading

Pull Request Test Coverage Report for Build 2385

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 62.446%
Totals Coverage Status
Change from base Build 2381: 0.0%
Covered Lines: 1021
Relevant Lines: 1635
💛 - Coveralls

@cgzones cgzones force-pushed the master_perlcritic4 branch from f170c72 to 4ac0cd0 Compare August 21, 2018 10:50
@cgzones cgzones force-pushed the master_perlcritic4 branch from 4ac0cd0 to 5478e5c Compare August 21, 2018 10:55
steveschnepp
steveschnepp requested changes Aug 25, 2018
View reviewed changes
Copy link
Member

@steveschnepp steveschnepp left a comment

Choose a reason for hiding this comment

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

Globally adding another round of warning is a very good idea.

I'm just not convinced on all the rules :)

lib/Munin/Node/SpoolWriter.pm
@@ -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 •
edited
Loading

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?

script/munin-httpd
@@ -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?

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

cgzones commented Sep 21, 2018

I looked over perlcritic severity 4 warnings and like to ask what to do about the following warnings:
* Modules::ProhibitAutomaticExportation
* Objects::ProhibitIndirectSyntax
* Subroutines::ProhibitBuiltinHomonyms
* Subroutines::RequireArgUnpacking
* Subroutines::RequireFinalReturn
fix or silence them?

any comments?

@sumpfralle
Copy link
Collaborator

@steveschnepp: what do you think about cgzone's "fix or silence" question?

@sumpfralle
Copy link
Collaborator

I am trying to clean up a bit ...

@cgzones: can we stick to const or is there a good reason for Readonly?
@steveschnepp: what do you think about #1051 (comment)?

@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
Reviewers

@sumpfralle sumpfralle sumpfralle left review comments

@steveschnepp steveschnepp steveschnepp requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Milestone
2.999.16
Development

Successfully merging this pull request may close these issues.
4 participants
@cgzones @coveralls @sumpfralle @steveschnepp