Workaround for PkgVersion/PerlCritic problems #27

Merged
merged 0 commits into from Nov 7, 2011

Projects

None yet

5 participants

@DarwinAwardWinner

It's kind of annoying that using Moose best practices and PkgVersion and Perl Critic together cause Perl Critic to complain about code before strict. There are ways to overcome this, but perhaps the easiest is to surround the package declaration with ## no critic and ## use critic. I've implemented this, controlled by an option "critic_workaround" which defaults to false. Setting critic_workaround = true should keep Perl Critic from complaining.

I don't know it it's possible to automatically detect whether Perl Critic is being used (by searching for CriticTests in the list of enabled plugins, perhaps?), but if it is, it would be preferable in the future to get rid of the config option and automatically detect when this feature should be enabled.

If you don't want this feature in core dzil, I can also release it as a separate module Dist::Zilla::Plugin::PkgVersion::CriticCompliant or something. Let me know which you prefer.

@dinomite

This would be nice to have!

@dolmen

I would be cleaner to put a single "## no critic" on the $VERSION line instead of the "no critic/use critic" block that uses more lines.

@rjbs
Owner
  1. This patch was never tested. It has a syntax error. Merging it breaks PkgVersion entirely. I fixed that and put it in my critic-workaround branch.
  2. Please don't send pull requests with merge commits in them. I can just ignore them, but it would be easier if you didn't include them, but rather rebased you work.
  3. There are no tests for the feature.

Please add tests and this can go in the next release.

Sorry if I sound negative, and sorry for the delay. I appreciate the time you took to produce this.

@dolmen

I suggest to rename the option to "no_critic".

@DarwinAwardWinner

Sorry about the merge commit. I believe I submitted this pull request while I was still learning the correct usage patterns of git. I now know how to use rebase to submit a proper pull request.

As for the lack of tests, this was also before I figured out how to write tests for dzil, which is a bit more involved than testing a typical Perl module. I vaguely remember testing this manually and having it work, but maybe something has changed since then that breaks it.

I'll see about adding tests and eliminating the merge commit when I have the time. Would it be better to just close this pull request and start a new branch off the critic-workaround branch in your repo (thereby effectively rebasing and eliminating the merge commit)?

@dolmen, I like the "no_critic" name for the option. I guess I was in a very literal mood when I originally wrote this. Regarding the use of a "no critic/use critic" block, I decided to go with the block method instead of the end-of-line because with the block method, I don't have to assume that the version declaration is only a single line. I believe that the version declaration does actually get split across two lines in some cases, so I figured it was safest just to surround it in a "no critic/use critic" block.

@rjbs
Owner

Hm. I don't know whether you can re-use this pull request for a new branch. I invite you to figure it out and do the right thing! :-D

@dolmen

@DarwinAwardWinner: $VERSION's definition is always on a single line. That is part of the informal specification of the CPAN toolchain implemented in, among others, ExtUtils::MakeMaker.

@DarwinAwardWinner

Even though the version declaration is on a single line, this plugin adds multiple lines of code (it surrounds the version declaration with a BEGIN block), so a single comment at the end of the line may not work. Also, in a trial release, a #TRIAL comment gets added on the version line, so adding a ## no critic comment on the same line might break something. So I think I will stick with the surrounding comments rather than trying to make an end-of-line comment work.

Now I just need to figure out how to test the feature. I guess I should first test that the requisite lines do indeed get added when the no_critic option is used. Is it possible to actually run Perl::Critic on the resulting code? This would add Perl::Critic and maybe Dist::Zilla::Test::Perl::Critic to the dependencies of Dist::Zilla. I'm not sure if that's appropriate. Can Dist::Zilla itself depend on non-core plugins?

@DarwinAwardWinner DarwinAwardWinner merged commit d14096f into rjbs:master Nov 7, 2011
@DarwinAwardWinner

I've submitted a proper pull request using topic branches instead of master. You can find it here: #69

@hartzell

This seems like it would be a really useful feature, I'm currently gnashing my teeth in preparation for wrapping all of my package lines with nocritic/critic.

Anything I can do to help it move forward? It looks as if there is a lot of stuff in this pull request that doesn't have anything to do with the feature. Would the process go faster if there was a pull request against a fork that only included the changes that implement the no_critic option?

@hartzell

It would be useful to be able to control the exact string that is set. It seems that I need to use

## no critic (TestingAndDebugging::RequireUseStrict, TestingAndDebugging::RequireUseWarnings)

otherwise I fall afoul of

[Miscellanea::ProhibitUnrestrictedNoCritic] Unrestricted '## no critic' annotation at line 1, near '## no critic '.  (Severity: 3)
@dolmen

Instead of trying to workaround Perl::Critic, did you try to fix it? If yes, please reference the rt.cpan.org tickets here.

@hartzell

[This thread is continuing here]

It's not really something fixable, that policy considers this kind of thing an error.

The only alternative (that I can think of) would be to author ::Lax versions of RequireUseStrict and RequireUseWarnings that agree that it is ok to have a VERSION assignment before some that sets strict and warning (e.g. Moose). That seems less clean than this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment