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

Address Perl::Critic violations #43

Closed
2 of 5 tasks
jonasbn opened this issue Jan 11, 2021 · 3 comments · Fixed by #178
Closed
2 of 5 tasks

Address Perl::Critic violations #43

jonasbn opened this issue Jan 11, 2021 · 3 comments · Fixed by #178
Assignees
@jonasbn
Labels
maintenance Maintenance of tools and configuration

Comments

@jonasbn
Copy link
Collaborator

jonasbn commented Jan 11, 2021 •
edited
Loading

  • Address the sections marked with ## no critic scattered throughout the code base, this might be valid, but another perspective and suggestions for refactorings might be a good idea (patches?)
  • follow up up Perl::Critic policies disabled and marked as TODO in the
    t/perlcriticrc file
  • Investigate whether use warnings pragma, breaks too much backwards
    compatibility [TestingAndDebugging::RequireUseWarnings]
  • Investigate violations of Perl::Critic policy [Subroutines::ProhibitExplicitReturnUndef]
  • Investigate violations of Perl::Critic policy [ValuesAndExpressions::ProhibitMagicNumbers]
@jonasbn jonasbn added the maintenance Maintenance of tools and configuration label Jan 11, 2021
ehuelsmann added a commit to ehuelsmann/perl-workflow that referenced this issue Jan 19, 2021
@ehuelsmann
Re perl-workflow#43: Enable Critic tests the code base already compli…
8c106aa 
…es with

The code base already complies with:

* [TestingAndDebugging::RequireUseWarnings]
* [ValuesAndExpressions::ProhibitMagicNumbers]
ehuelsmann added a commit to ehuelsmann/perl-workflow that referenced this issue Jan 19, 2021
@ehuelsmann
Re perl-workflow#43 address 'no critic' sections
588bf4d 
Using Perl::Critic 1.138, `dzil test` succeeds - that is including
perlcritic tests - after this commit, so without the 'no critic'
sections.
jonasbn pushed a commit that referenced this issue Jan 22, 2021
@ehuelsmann
Re #43: Enable Critic tests the code base already complies with (#58)
028dfab 
The code base already complies with:

* [TestingAndDebugging::RequireUseWarnings]
* [ValuesAndExpressions::ProhibitMagicNumbers]
jonasbn pushed a commit that referenced this issue Jan 22, 2021
@ehuelsmann
Re #43 address 'no critic' sections (#59)
d15d9ac 
Using Perl::Critic 1.138, `dzil test` succeeds - that is including
perlcritic tests - after this commit, so without the 'no critic'
sections.
@jonasbn jonasbn self-assigned this Jan 22, 2021
@ehuelsmann
Copy link
Member

@jonasbn with some of the PRs addressed, could you check off the check marks you consider completed, or detail what's left for any that I PR-ed for?

@jonasbn
Copy link
Collaborator Author

jonasbn commented Dec 11, 2021 •
edited
Loading

Ok, we still disable strictures in a few places as reported by TestingAndDebugging::ProhibitNoStrict

Stricture disabled at lib/Workflow/Factory.pm line 31
Stricture disabled at lib/Workflow/Factory.pm line 287
Stricture disabled at lib/Workflow/Factory.pm line 546
Stricture disabled at lib/Workflow/Config/Perl.pm line 84
Stricture disabled at lib/Workflow/Persister/File.pm line 155

@jonasbn
Copy link
Collaborator Author

jonasbn commented Dec 11, 2021

We should not have any issues with [TestingAndDebugging::RequireUseWarnings] since we are requiring Perl 5.14

jonasbn added a commit that referenced this issue Dec 11, 2021
@jonasbn
Addressed several Perl::Critic violation, several disabled project sc…
03f6d57 
…ope in rc file other locally, more work to be done, but this addresses somewhat issue #43 (closes #43)
@jonasbn jonasbn mentioned this issue Dec 11, 2021
Merged
5 tasks
@jonasbn jonasbn linked a pull request Dec 11, 2021 that will close this issue
Addressed several Perl::Critic violation, several disabled project scope in rc file other locally, more work to be done, but this addresses somewhat issue #43 (closes #43) #178
Merged
5 tasks
jonasbn added a commit that referenced this issue Jan 24, 2022
@jonasbn
Addressed several Perl::Critic violations
b6ce54a 
* Addressed several Perl::Critic violation, several disabled project scope in the rc file other locally, more work to be done, but this addresses somewhat issue #43 (closes #43)

* Reverted some of the changes, they did not feel right - too excessive use of constants

* Updated relevant comments
jonasbn added a commit that referenced this issue Jan 27, 2022
@jonasbn
Addressed several Perl::Critic violation, several disabled project sc…
090affb 
…ope in rc file other locally, more work to be done, but this addresses somewhat issue #43 (closes #43)
@jonasbn jonasbn mentioned this issue Feb 11, 2023
Closed
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@jonasbn jonasbn

Labels
maintenance Maintenance of tools and configuration
Projects
None yet
Milestone
No milestone
2 participants
@jonasbn @ehuelsmann