New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Perl::Critic #45

Merged
merged 2 commits into from Jun 7, 2015

Conversation

Projects
None yet
2 participants
@VadimPushtaev
Contributor

VadimPushtaev commented Jun 1, 2015

Hi.

I believe Strehler is pretty big project to have its own Perl::Critic system. However, as long as I'm not the author, I don't really know what you consider good or bad.

So I tried to create some scaffold for you. You can merge it as-is, merge and edit, ask me to change what you want or even don't merge it at all and use it as a inspiration for you own Perl::Critic system :).

I also fixed some problems Perl::Critic showed me.

P. S. I got you repo for Pull request challenge this month.

@cym0n

This comment has been minimized.

Show comment
Hide comment
@cym0n

cym0n Jun 2, 2015

Owner

Hi, great work and thanks for your time spent on Strehler!

I pushed a branch named merge_45, started from yours, if you want to go on collaborating on the issue. What I did on it:

  • Run perlcritic also on test directory t/ and script/strehler. Probably it's useless on test, but I think it's good to have in on the management script, I found a lot of stuff worth of some work. (TODO just for me: strehler script must be deeply tested)
  • Added perlcritic to my Dist::Zilla configuration. Doing that, I turned off TestingAndDebugging::RequireUseStrict because in its opinion Dist::Zilla add module version in the wrong place :-)

Unfortunatly there's still an open issue. Changing eval "require $class" to load $class broke something, test t/006_extra.t returns now a lot of strange errors. I'm trying to find out how to fix it. If I will have to give up, i'll revert that modification excluding the code from critic

Owner

cym0n commented Jun 2, 2015

Hi, great work and thanks for your time spent on Strehler!

I pushed a branch named merge_45, started from yours, if you want to go on collaborating on the issue. What I did on it:

  • Run perlcritic also on test directory t/ and script/strehler. Probably it's useless on test, but I think it's good to have in on the management script, I found a lot of stuff worth of some work. (TODO just for me: strehler script must be deeply tested)
  • Added perlcritic to my Dist::Zilla configuration. Doing that, I turned off TestingAndDebugging::RequireUseStrict because in its opinion Dist::Zilla add module version in the wrong place :-)

Unfortunatly there's still an open issue. Changing eval "require $class" to load $class broke something, test t/006_extra.t returns now a lot of strange errors. I'm trying to find out how to fix it. If I will have to give up, i'll revert that modification excluding the code from critic

@cym0n cym0n merged commit 696a813 into cym0n:master Jun 7, 2015

@cym0n

This comment has been minimized.

Show comment
Hide comment
@cym0n

cym0n Jun 7, 2015

Owner

All the problems solved!
Branch merged!
Thanks a lot again!

Owner

cym0n commented Jun 7, 2015

All the problems solved!
Branch merged!
Thanks a lot again!

@VadimPushtaev

This comment has been minimized.

Show comment
Hide comment
@VadimPushtaev

VadimPushtaev Jun 8, 2015

Contributor

What about that load? What was the problem?

Contributor

VadimPushtaev commented Jun 8, 2015

What about that load? What was the problem?

@cym0n

This comment has been minimized.

Show comment
Hide comment
@cym0n

cym0n Jun 8, 2015

Owner

There was a (useless) require in the test script. That plus the Module::Load call made the module been loaded twice, a really bad thing from interpeter point of view!

Here's the commit that fixed: bbdc11a#diff-3856c4a784b9f9ecd515edade68c1b1c

(probably I should get rid of the other require too... but it's working all well right now and I don't want to touch it too much ;-)

Owner

cym0n commented Jun 8, 2015

There was a (useless) require in the test script. That plus the Module::Load call made the module been loaded twice, a really bad thing from interpeter point of view!

Here's the commit that fixed: bbdc11a#diff-3856c4a784b9f9ecd515edade68c1b1c

(probably I should get rid of the other require too... but it's working all well right now and I don't want to touch it too much ;-)

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