Grinnz / Perl-Critic-Community

Perl::Critic::Community - Community-inspired Perl::Critic policies
https://metacpan.org/pod/Perl::Critic::Community
Other
8 stars 10 forks source link

The StrictWarnings policy seems to reimplement policies from the Perl::Critic core #8

Closed autarch closed 9 years ago

autarch commented 9 years ago

https://metacpan.org/pod/Perl::Critic::Policy::TestingAndDebugging::RequireUseStrict and https://metacpan.org/pod/Perl::Critic::Policy::TestingAndDebugging::RequireUseWarnings

If your policy doesn't do anything different, I'd recommend removing it. If it does, please document the differences.

Grinnz commented 9 years ago

It treats many additional modules such as Modern::Perl and Moo as solving the "use strict/use warnings" requirement by default for one; I could list the defaults if that's helpful. Regardless, I cannot modify the core Perl::Critic policies to add them to this theme, so the policy needs to exist anyway.

autarch commented 9 years ago

Regardless, I cannot modify the core Perl::Critic policies to add them to this theme, so the policy needs to exist anyway.

Sure you can. Submit a PR. Perl::Critic has its own list of equivalent modules, and I'm sure the maintainer would welcome updates.

oalders commented 9 years ago

ping @thaljef

Grinnz commented 9 years ago

I think you misread: add them to this theme, as in the theme for this collection of policies, 'freenode'.

oschwald commented 9 years ago

Perl::Critic recently included "Moose, Moo, Mouse, Dancer, Mojolicious, and several other modules as equivalent to the strict and warnings pragma". This included Modern::Perl as well.

Grinnz commented 9 years ago

To note some other differences: it is placated as long as the appropriate "use"s show up somewhere in the source, because this policy is mostly designed to complain about code written by people who didn't know strict and warnings exist/need to be used, or to indicate to experienced perl programmers when they forgot to include them. Also, (minor) it's combined in one policy instead of separate, and thus only considers modules that import both (which is the majority of them anyway).

autarch commented 9 years ago

The only ones missing from Perl::Critic core right now are strictures and Any::Moose, I think. Meanwhile, this distro is missing several from PC core. Clearly not a great situation.

As for the theme issue, I think it'd make more sense to simply recommend that people use this theme in addition to others (themes = freenode && foo) Otherwise this theme would have to expand to cover more and more of the core PC functionality.

I think the PC design for themes isn't ideal, since it basically encourages you to duplicate functionality, but again, that's something to fix in the core. I think the right way to do this is to have the theme definition happen outside of the individual policies, so people can define themes by specifying modules. Maybe something like this:

package Perl::Critic::Freenode;

use Perl::Critic; Perl::Critic->add_theme( freenode => [ 'Policy1', 'Policy2', ... ] );

But that's just totally off the cuff.

Grinnz commented 9 years ago

I agree, it would be nice to be able to define a "theme module" which then includes additional policies "against their will" so to speak. But as it is, I am not willing to make the usage of this distribution more complex for newcomers.

EDIT: s/themes/policies/

Grinnz commented 9 years ago

As @STRICT_EQUIVALENT_MODULES and @WARNINGS_EQUIVALENT_MODULES are available from Perl::Critic::Utils::Constants, I will add the contents of these arrays to the recognized modules to hopefully help with the overlap issues.

thaljef commented 9 years ago

Send me a pull request and I'll be happy to add other strict-like and warnings-like modules to the list in Perl::Critic.

Grinnz commented 9 years ago

https://github.com/Grinnz/Perl-Critic-Freenode/commit/1e8aef6cca0cf7283a94b7d80f611c89655df0c9

Please reopen if this does not resolve your concerns.