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

Dist Updates for PR Challenge #3

Merged
merged 4 commits into from Feb 4, 2017

Conversation

Projects
None yet
2 participants
@genio
Contributor

genio commented Aug 13, 2016

Hi!

I received your dist as part of this month's CPAN Pull Request Challenge.

I made almost zero actual coding changes to your dist. So, while this seems like a rather large PR (for which I apologize) there is next to nothing in terms of actual code changes.

Code changes:

  • Got rid of indirect-object-syntax. I changed new Foo to Foo->new and import Foo to Foo->import.
  • I standardized your $VERSION declarations across your .PM files, adhering to Version Numbers Should be Boring.

Test Suite:

  • Added some boiler-plate tests in xt/author and xt/release that are solely for you, as the developer, to run and make sure your dist is in as good of shape as it can be. prove -lr xt will run these for you.
  • Added a little boiler-plate testing in t. I also moved your test files out of there and into xt/author since they weren't mentioned in the MANIFEST file before, they weren't being run, so I figured you wanted it that way.
  • Moved test.pl to t/02-coms.t. This works just as it did before, but now uses Test::More instead of rolling your own testing suite.
  • Made all tests make use of Test::More.
    • perl Makefile.PL
    • make test OR prove -lr t (neither of these will run tests in the xt/ dir)
    • prove -lr xt for optional tests

Build:

  • Updated the Makefile.PL a bit to include the prereqs that were missing before.
  • Updated the MANIFEST to include all of the files in the distribution
  • Updated the .gitignore to no longer worry about build files

genio added some commits Aug 9, 2016

@cosimo

This comment has been minimized.

Show comment
Hide comment
@cosimo

cosimo Jan 5, 2017

Owner

This is an awesome piece of work. Thank you!
I apologize that it took me so long to get to it, but I never saw this before today...

Owner

cosimo commented Jan 5, 2017

This is an awesome piece of work. Thank you!
I apologize that it took me so long to get to it, but I never saw this before today...

Show outdated Hide outdated Makefile.PL
use warnings;
use Test::More tests => 6;
BEGIN {

This comment has been minimized.

@cosimo

cosimo Jan 5, 2017

Owner

I assumed the use of use_ok() was deprecated a long time ago. See http://blogs.perl.org/users/ovid/2012/04/avoiding-use-ok-in-t00-loadt.html for example. Is it considered ok?

@cosimo

cosimo Jan 5, 2017

Owner

I assumed the use of use_ok() was deprecated a long time ago. See http://blogs.perl.org/users/ovid/2012/04/avoiding-use-ok-in-t00-loadt.html for example. Is it considered ok?

@cosimo

This comment has been minimized.

Show comment
Hide comment
@cosimo

cosimo Jan 5, 2017

Owner

I've added a few comments to the relevant sections of the code, but want to thank you again for this work. Very appreciated.
Tests are failing, but just because travis hasn't been configured. Would you be willing to add a default .travis-ci.yml in place so as to make the test suite run correctly? That would be great.

Owner

cosimo commented Jan 5, 2017

I've added a few comments to the relevant sections of the code, but want to thank you again for this work. Very appreciated.
Tests are failing, but just because travis hasn't been configured. Would you be willing to add a default .travis-ci.yml in place so as to make the test suite run correctly? That would be great.

@genio

This comment has been minimized.

Show comment
Hide comment
@genio

genio Jan 6, 2017

Contributor

Oops. I fixed the copy-pasta error on the Makefile. I added the Travis config and it appears to be testing correctly.

As for the load.t, I really don't have a strong argument for or against. I mean, the compile tests are there in xt/author, so I could remove that test completely?

Contributor

genio commented Jan 6, 2017

Oops. I fixed the copy-pasta error on the Makefile. I added the Travis config and it appears to be testing correctly.

As for the load.t, I really don't have a strong argument for or against. I mean, the compile tests are there in xt/author, so I could remove that test completely?

@cosimo

cosimo approved these changes Feb 4, 2017

@cosimo cosimo merged commit 8fcf1c2 into cosimo:master Feb 4, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@cosimo

This comment has been minimized.

Show comment
Hide comment
@cosimo

cosimo Feb 4, 2017

Owner

Very thorough improvement work. Thank you very much!

Owner

cosimo commented Feb 4, 2017

Very thorough improvement work. Thank you very much!

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