Instantiate Perl::Critic only once #11
Conversation
|
Slick. I like it. |
lib/Test/Perl/Critic.pm
Outdated
|
|
||
| my $pc; | ||
| my $critic = sub { | ||
| my ( %args ) = @_; |
There was a problem hiding this comment.
Maybe it would be wise to singletonize the %args too, so that you don't get differently inited Critic instance after calling this with arguments different from the ones in the first call.
There was a problem hiding this comment.
@Dominik-Sauer I haven't exactly singletonize %args, but rather reset the possibly initialized P::C object with each call to import. I guess that solves the issue as well.
| @@ -40,22 +47,16 @@ sub import { | |||
| if ( exists $args{-format} ) { $args{-verbose} = $args{-format}; } | |||
| %CRITIC_ARGS = %args; | |||
|
|
|||
| # reset possibly lazy-initialized Perl::Critic | |||
| $CRITIC_OBJ = undef; | |||
There was a problem hiding this comment.
Without this commit, this might have happened:
Test::Perl::Critic->import( -profile => 'profile-A' );
critic_ok('source.pm');
# now $critic is initialized to P::C instance referring to profile-A
Test::Perl::Critic->import( -profile => 'profile-B' );
# here $critic hold the reference to P::C instance referring to profile-A
critic_ok('source.pm')
# the result might not be what you would expect
lib/Test/Perl/Critic.pm
Outdated
| @@ -23,6 +23,13 @@ my $TEST = Test::Builder->new; | |||
| my $DIAG_INDENT = q{ }; | |||
| my %CRITIC_ARGS = (); | |||
|
|
|||
| my $CRITIC_OBJ = undef; | |||
| my $GET_CRITIC = sub { | |||
There was a problem hiding this comment.
don't use 'get' - my $BUILD_CRITIC
implementation:
return $CRITIC_OBJ //= Perl::Critic->new( @_ )There was a problem hiding this comment.
ok, my fault, apparently it should work also for perl < 5.10
still, assign to %args is useless
There was a problem hiding this comment.
@branislav-zahradnik-gdc Ok, I'll change that.
|
I squashed couple of commits into one, since one without the others wouldn't make sense. The change is pretty small, anyway. |
|
I'd like to get this merged, but the test fails under Travis. Can you please look into it? |
|
And, are we sure there's no ill effects of reusing the singleton? |
|
Can I close this and let you create a new PR when you've got it going? |
|
No problem, let's close it for now. |
|
Hi @petdance , any chance this could be merged in provided the tests has been fixed previously? Thanks! |
|
@petdance: can you look at this change? |
|
Done. It's in the just-pushed-to-CPAN 1.04. Thanks for the nudge. |
|
@petdance Thanks! |
No description provided.