stevan / p5-App-Critique

An incremental refactoring tool for Perl powered by Perl::Critic
3 stars 4 forks source link

Moved the file filters into their own plugin. #11

Closed stuartskelton closed 8 years ago

stuartskelton commented 8 years ago

The Plugin version of #8. Its pretty much a carbon copy with just a few things moved about.

Except for a tinnie tiny bug I noticed with respect to _filter, if the default or fallback subroutine requires more than just $opt to be passed in, like no-violations, then it would have failed. Lucky it was a noisy fail, but a fail none the less.

I am starting to get the feeling I want to use named params when dealing with the file filters, as it would remove some ambiguity on what param goes where.

stevan commented 8 years ago

Haha, I should read all the pull requests before commenting. Let me look over this one and I will comment inline.

stevan commented 8 years ago

So, I like the idea of named arguments, and I hacked up a quick version of the regex one to show what I mean. This also allows us to avoid passing in the $opt and instead unpack it into args more specific (see below), it also allows us to have the success and failure handlers (again see below).

sub _regex {
    my (%args) = @_;
    return sub {
        return unless $args{filter};
        my $path = $_[0]->path->stringify;
        my $f = $args{filter};        
        my $is_match = $args{invert} ? $path !~ /$f/ : $path =~ /$f/;
        if ( $args{verbose} ) {
            if ($is_match) {
                $args{success}->( $path );
            }
            else {
                $args{failure}->( $path );
            }
        }
        return !!$is_match;
    };
}

Hopefully this makes sense, it should be a relatively straightforward tweak, let me know if you run into issues or have questions.

stevan commented 8 years ago

And if I didn't say it before, thank you very much for this work, it is much appreciated.

stuartskelton commented 8 years ago

The call back method is a very nice idea, it could make building composite filters, but I would have to think about it a little more.

so a rough thought. So we could (it my brain has it right, ps I am dyslexic), we could combine the filter and no-violations, so we could prune only test files with violations, .. or we could be not lazy and prune twice, first on file then with -no-violations.

stevan commented 8 years ago

So I was thinking something similar, that the filter step in collect is fairly redundant with the entire prune command.

I think perhaps the better approach is to add a --no-violations switch to the collect command so that is can be filtered immediately during collection.

Then perhaps add a --prune switch to the collect command which allows you to further filter an existing list of files (basically folding the entire functionality of the prune command into collect).

This is perhaps starting to become something larger in scope, so I am willing to spend some time on it as well, or I can leave it to you to work on if you like.

stuartskelton commented 8 years ago

Leave it with me, what I might do, is get this up to scratch then move on to that next as it will give me a clean spring board to move on whats next.

stevan commented 8 years ago

That sounds like a good plan to me.

stuartskelton commented 8 years ago

Was super busy at work, here is the latest code.

I took a fair amount of inspiration of the sample code you knocked up, and it works nicely.

stevan commented 8 years ago

Okay, I have merged this, I might make a few changes here and there, but overall it looks great to me. Much thanks for the work and effort :)