Critic workaround, now with tests and an actual working implementation #69

Closed
wants to merge 51 commits into from

6 participants

@DarwinAwardWinner

Much better than last time.

dolmen and others added some commits Oct 30, 2011
@dolmen dolmen Optimize Chrome::Term initial loading
Load the following modules only when used:
- Term::ReadKey
- Term::ReadLine (and its dependencies such as Term::ReadLine::Gnu)
- Term::UI
- Encode
This optimizes runtime for most use cases (DZ commands that do not
require user interaction).
8e10b0b
@dolmen dolmen Optimize Dist::Zilla::Util startup
Load Pod::Eventual at runtime only if abstract_from_file is used.
This improves 'dzil authordeps' speed.
33f59d7
@dolmen dolmen DZ::App: don't load Path::Class (not used here) 0cdade9
@dolmen dolmen DZ:Util: load Path::Class and File::HomeDir on demand
In Dist::Zilla::Util, load Path::Class and File::HomeDir only when
_global_config_root is called.
3e12693
@dolmen dolmen DZ:A:C🆕 don't load Path::Class (not used here) 4705317
@dolmen dolmen DZ:A:C:setup: don't load Path::Class (not used here) 294607a
@dolmen dolmen DZ:A:C:authordeps: load Path::Class only when used 4c85cf9
@madsen madsen Add a before_release check for missing credentials 11b856c
@madsen madsen Also abort if 0-length username/password
  Mention prompting behavior in docs
1e49d5f
@madsen madsen Add tests for UploadToCPAN 976548e
@madsen madsen Use Test::Fatal in fakerelease.t
With Try::Tiny, if the code fails to die as expected, the catch block is never
executed and the test is just skipped (instead of failing as it should).  Since
there's no plan, the test script gets a PASS.  You're unlikely to notice that
it didn't run as many tests as it should.
583f9d4
@madsen madsen Add extra_scanners & scanners to AutoPrereqs 399ac4c
@madsen madsen Filter minimum perl version out of $build_prereq
  MakeMaker doesn't understand it
  When setting $perl_prereq, look in both runtime requires and build requires.
74ecf70
@madsen madsen Test that minimum Perl version is pruned correctly 86ba7fa
@madsen madsen Increment Test::Builder::Level in test functions
Failures should report the line number in the test script,
not in Test::DZil.
6f6fd35
@madsen madsen is_filelist accepts Dist::Zilla::Role::File objects b21a18e
@madsen madsen Add FileFinder::ByName and ffbyname.t c9c4450
@madsen madsen More tests for FileFinder::ByName a4a2560
@rjbs (Thanks, Christopher J. Madsen!) 9a3f3f8
@rjbs do some obsessive formatting tweaking fec907f
@rjbs v4.300003
          UploadToCPAN will now prompt for credentials during release, if none
          can be found on disk (Thanks, Christopher J. Madsen!)

          AutoPrereqs can now be told what scanners or extra_scanners args to
          pass to Perl::PrereqScanner (Thanks, Christopher J. Madsen!)

          Prune perl prereqs out of BUILD_REQUIRES for EU::MM, and pick the
          highest required perl of either runtime or built requires.  (Thanks,
          Christopher J. Madsen!)

          Test routines should now properly report the calling line for
          failures ($Test::Builder::Level is being set) (Thanks, Christopher J.
          Madsen!)
dc5cbff
@rjbs fix changelog for last release 6891389
@rjbs Merge remote-tracking branch 'dolmen/faster-startup' ed393c6
@rjbs convert more "use" to "require" in dzil authordeps 16751ac
@rjbs load fewer modules at startup for listdeps 0d75f01
@rjbs avoid unneeded module loading in dzil clean 93251c7
@rjbs avoid unneeded module loading in dzil new 5d7e5ff
@rjbs avoid more unneeded module loading in dzil run d14096f
@DarwinAwardWinner

One potential additional feature here would be auto-detection of commonly-used modules that run your code through Perl Critic. In such cases, no_critic would automatically be set to 1 if it is not already set. Is it possible for a plugin to get a list of all enabled plugins? I realize that in general it is probably bad design for plugins do do things conditionally based on the presence or absence of other plugins, but in this case the difference is nothing more than two extra comment lines, so I think such behavior would be appropriate.

madsen and others added some commits Nov 11, 2011
@madsen madsen Add FileFinder::Filter
  Add its tests to ffbyname.t, because it needs results from another finder,
  and failures in that finder will cascade anyway
1030eda
@dolmen dolmen Test more abstract_from_file
Modify corpus/dist/DZ2/DZ1.pm to set the abstract from POD instead of an
"# ABSTRACT:" line. This improves a bit the code coverage of DZ::Util.
bd88607
@rjbs document the "why" of the last commit 6b3c18d
@DarwinAwardWinner DarwinAwardWinner Add critic_workaround to PkgVersion plugin
If you set critic_workaround to true, then the version declaration
will be surrounded by "## no critic" and "## use critic" so that
Perl::Critic doesn't complain about it if the package is declared
before "use strict".
5d8bd7e
@DarwinAwardWinner DarwinAwardWinner Fix syntax and add comments explaining what is happening. f8fc3e2
@DarwinAwardWinner DarwinAwardWinner Rename critic_workaround to no_critic 7000f4c
@DarwinAwardWinner DarwinAwardWinner Fix comment f8092c7
@DarwinAwardWinner DarwinAwardWinner Add documentation for no_critic 56523c3
@DarwinAwardWinner DarwinAwardWinner Minor whitespace fix in generated code 1d9e4a3
@DarwinAwardWinner DarwinAwardWinner Fix PPI code to properly handle no_critic comments 27ae0fd
@DarwinAwardWinner DarwinAwardWinner Add tests for PkgVersion no_critic=1 5a98aa8
@rjbs
Owner

I'd like to merge this soon. Can you tell me why it generates:

## no critic
our $VERSION = ...
## use critic

instead of

our $VERSION = ... ## no critic

?

@DarwinAwardWinner

Because the version line can already have # TRIAL appended to it, and I don't know if our $VERSION = XXX # TRIAL ## no critic would work correctly. I can modify it to put ## no critic at the end of the version line if $trial is an empty string, and use the separate-line comments otherwise.

@DarwinAwardWinner DarwinAwardWinner Minor code cleanup (no functional change) 0aa1b6f
@DarwinAwardWinner DarwinAwardWinner Better interaction between trial and nocritic
The nocritic in PkgVersion option will add an end-of-line '## no
critic' comment, unless the version is a trial version. In that case
the end-of-line comment is '# TRIAL', so the nocritic option puts '##
no critic' and '## use critic' on surrounding lines instead.

Tests are updated to reflect this behavior.
7e21748
@DarwinAwardWinner DarwinAwardWinner Remove extraneous braces from generated code 94efd7b
@DarwinAwardWinner DarwinAwardWinner Complete rewrite of PkgVersion nocritic tests 6765617
@DarwinAwardWinner DarwinAwardWinner Actually ensure "nocritic" PkgVersion code is critic-compliant
Extra tests actually run Perl::Critic on code generate with nocritic
mode enabled. The tests fail if the Perl::Critic complains. There is
also a "positive control" with nocritic disabled.
914f0ba
@DarwinAwardWinner

Ok, fixed. If $ENV{TRIAL} is false, then you get something like:

our $VERSION = ...; ## no critic

If $ENV{TRIAL} is true, then you get something like:

## no critic
our $VERSION = ...; # TRIAL
## use critic

I also added tests that will use Perl::Critic to actually verify that the generated code is indeed critic-compliant (instead of just verifying that the comments are added).

A note: I rebased this onto rjbs/master a short time ago, so it is no longer branched off of rjbs/critic-workaround. In doing so, I resolved a few merge conflicts that you now won't have to deal with. Unfortunately, the rebase confused GitHub, so that's why you're seeing a bunch of commits from the master branch listed here.

@DarwinAwardWinner

I've added autodetection for DZP::Test::Perl::Critic. Now, if your config includes a [Test::Perl::Critic] section, PkgVersion will enable no_critic by default. Tests are included.

By the way, if you want me to squash this branch's history into more logical commits before you merge (instead of "what I did in the order I did it" commits), I'm happy to do so. Just let me know.

@DarwinAwardWinner

Hi @rjbs, are you still interested in merging this? I think I've addressed all the concerns that you brought up, and as far as I'm concerned, this is ready to go. It should just work without any additional user configuration, since it automatically detects the stock dzil critic plugin.

@karenetheridge

Looks like you need to rebase against the latest rjbs/master, to remove all the other commits from other people that don't belong. You can git push --force into the same branch, and this pull request will be updated automatically.

@hartzell

[I thought I'd commented on this earlier this evening, but I don't see my "contributions", so I'll try again]

I'd like to see this happen. I suspect that providing a pull request from a fork/branch (I'm new here...) that only contained the changes that implement/support this feature would make it much easier for rjbs to integrate them.

I actually need to use

## no critic (RequireUseStrict, RequireUseWarnings)

otherwise I run afoul of

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

But otherwise the approach seems great.

@karenetheridge

@DarwinAwardWinner - it looks like you need to rebase your branch against the latest master. There shouldn't be all those extra commits listed in this pull request - github can figure out what commits are present in master, and only list what is new in your branch.

(Alternatively, you can create a new branch off of master, cherry-pick your commits there, squash them down, and open a new pull request that references this one.)

@rjbs
Owner

The idea of putting in "no critic" seems pretty gross.

There are at least two policies at play, right?

  1. don't put anything before package
  2. don't put anything before strict and warnings except for package

I don't like the first one, because it's easier to just say use strict; use warnings; package X;

That's why I wrote the Lax::RequiresExplicitPackage::ExceptForPragmata policy. It looks like the allow_import_of option to the default RequiresExplicitPackage policy now lets you handle this.

So this is about the second one, right? We don't want to assign to the version until we've turned on strictures.

If you're really using strict and warnings directly, and before the package statement, there is no problem here. If your code looks like:

use strict;
use warnings;
package X;

…
1;

...then PkgVersion will put the VERSION on line 4, after strict, warnings, and the package. The problem case is when you're getting strictures from something that provides them by proxy, like Moose.

package X;
use Moose;

 ....
 1;

Here, the version will go on line 2, and there really aren't strictures.

So, I wrote 4b75104, which tries to put the VERSION after use statements.

There are at least two problems:

  • as the test demonstrates, the comment is not being kept in place properly; I fought with this for a bit and gave up; it needs more PPI finesse, I think
  • if anything being use-d was hoping to see VERSION set, it won't... but that shouldn't be much of a problem, anyway, since the VERSION assignment was effectively later than the use because of the BEGIN implicit on use -- but probably the "skip Include statements" will need to stop skipping early on a require, which is not BEGIN-phase
@DarwinAwardWinner

All the extra unrelated commits are because I submitted this pull request against the critic-workaround branch in @rjbs's repo, as requested in #27 here, and then later rebased onto master. (Although re-reading it now, I'm not really sure if that's what @rjbs was asking for, so now I'm just wondering what my past self was thinking a year ago.) It doesn't look like there's a way to change the "merge target" of a pull request after submitting it, so I guess I would have the submit a new one against master?

Anyway, of course I'm happy to rebase onto the latest master if you decide to pursue this approach to solving the problem. However, it seems @rjbs wants to do something less hackish by placing the version intelligently, which means my code may not be needed, although the tests might still come in handy.

Let me know what you want.

@DarwinAwardWinner

@hartzell, I've implemented your change. I'm holding off on pushing it until we decide whether to rebase or start over.

@hartzell

@rjbs I'm not sure (and my PPI-fu is still weak) how far into the document you're considering parsing when you say you want to put it "after the use statements". Rather than walking goodness knows how far into the document could we scan forward until we've seen a strict and a warnings (or something from RequireUseStrict's equivalent_modules list (hard code it and allow for an option to customize)) and drop it there. If we can't find a good place then drop it right after the package statement and watch the fireworks?

@DarwinAwardWinner

I think the idea is to start at the package statement and walk forward until you hit the first statement that isn't a use, and put the version right before that.

@rjbs
Owner

Yes, the problem is, as I said, that comments get separated from their homes. You write:

package X;
use Moose 2.001; # needed for the metaconfabulator

...and PkgVersion edits:

package X;
use Moose 2.001;
{ our $VERSION = 1 }
# needed for metaconfabulator

If this bug can be fixed in the commit I mentioned above (in the branch critic-version) I would be happy with it.

@DarwinAwardWinner

I think this can be solved as follows: we look at the final use statement along with all the following PPI::Elements on the same line (as given by PPI::Element->line_number). If any of the following statements are significant (PPI::Element->significant), then push them all to a new line after the version declaration. If they are all insignificant, put the version declaration after all of them on the next line. I'll try to implement this.

@DarwinAwardWinner

I have an apparently working implementation here: fe948cb

However, I had to do some evil. In order to insert the version declaration, I had to insert a PPI::Token::Whitespace and then call add_content on it to append the version declaration, which is a questionable decision which probably fails to guarantee a syntactically correct result. See line 150-156 og PkgVersion.pm

So right now it's a proof of concept that passes the test (that I added), so please feel free to take a look at it, but I wouldn't merge it yet.

(Also, the code is really messy and would need major cleanup before merging, but that's a secondary concern.)

@hartzell

As I ready myself to add no critic comments to 23 files in a new-ish project, I wonder if there is anything (more) that I can do to get this ball rolling (again)?

@DarwinAwardWinner Do you see yourself moving forward with your patch above?

@DarwinAwardWinner

Well, I've pushed my PPI-fu as far as it can go. As I mentioned, my current implementation uses an ugly hack that likely doesn't guarantee a result that is syntactically correct. Basically, I would need someone who actually knows how to use PPI to reimplement the same logic from scratch, or at least give me a hint on how to do it.

In any case, you can apply my patch to your local copy of Dist::Zilla and use it without waiting, if you want to.

@hartzell

I have something that should work, but doesn't. I think there might be a PPI bug (some code may have diverged by cut and paste) and have contacted Adam Kennedy about it (can't find the Parse-Perl mailing list).

As noted above, Ricardo's problem is that his first cut at walking over any PPI::Statement::Include's ends up tearing a trailing comment off of something like

use Moose 2.001; # for nebulizer support

It turns out that the released PkgVersion tears comments off of the package statement...

This:

package MooX::StrictConstructor; # some comment goes here

becomes this:

package MooX::StrictConstructor;
{
  $MooX::StrictConstructor::VERSION = '0.005';
} # some comment goes here

If I can figure out what's going on with my spin on Ricardo's hack I can clean that up too.

I'll give Adam a bit to respond and see what's up.

@hartzell

See this pull request for a [reasonably] clean and working implementation of RIcardo's approach, with tests.

@rjbs
Owner

closed in favor of #168

@rjbs rjbs closed this Jul 25, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment