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

CPANPR challenge #3

Open
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@doyleyoung

doyleyoung commented Mar 26, 2015

I received your module as my assignment for March from the CPAN Pull Request Challenge. As I reviewed the code and documentation the only thing that really stood out to me was the strong difference in formatting between Packlist.pm and the rest of the code. So, I reformatted it so it's style was the same as the other code. You had one easily avoidable CPANTS Kwalitee warning about a LICENSE file, so I added that, too.

Thank you for doing your part to make Perl amazing.

@Leont

This comment has been minimized.

Member

Leont commented Mar 26, 2015

I'm not that much a fan of formatting patches, specially when a file is consistent with itself, but one can argue in favor of it.

I don't think that's a proper license file. It really should contain all the legalese (the entire license), not just "you can redistribute it and/or modify it under the same terms as Perl itself".

@doyleyoung

This comment has been minimized.

doyleyoung commented Mar 26, 2015

Packlist.pm was using both tabs and spaces for indentation, and it had inconsistent formatting of if, for and while braces. I thought it was enough to justify a change.

Would you like me to add the full license to the LICENSE file? The terms of Perl are here, do you think it's ok to just include links or should the full text of both the GPL and the Artistic License be included?

@karenetheridge

This comment has been minimized.

Member

karenetheridge commented Mar 26, 2015

I'd support a patch to Makefile.PL that generated the LICENSE using App::Software::License -- or otherwise, just generate it manually and commit the file (since its content rarely changes).

@Leont

This comment has been minimized.

Member

Leont commented Mar 26, 2015

Packlist.pm was using both tabs and spaces for indentation, and it had inconsistent formatting of if, for and while braces. I thought it was enough to justify a change.

Mixed tabs and spaces the traditional indentation in perl core, except the authors of EU::I didn't introduce an indentation level for a function scope. I'd agree your indentation is preferable, but the old one was not inconsistent just unusual.

Would you like me to add the full license to the LICENSE file? The terms of Perl are here, do you think it's ok to just include links or should the full text of both the GPL and the Artistic License be included?

Yes, it is customary (and actually useful) to include the full license text(s)

@doyleyoung

This comment has been minimized.

doyleyoung commented Mar 27, 2015

I added the full license text to the LICENSE file. @karenetheridge thanks for the A::S::L recommendation, it worked great.

@mohawk2

This comment has been minimized.

Member

mohawk2 commented Mar 27, 2015

@doyleyoung, if you're feeling energetic, feel like having a go at Perl-Toolchain-Gang/ExtUtils-MakeMaker#203 (and connected, Perl-Toolchain-Gang/ExtUtils-MakeMaker#204)? If you want to chat through the issues, #toolchain on irc.perl.org is where the PTG folks hang out and will be pleased to help.l

@dolmen

This comment has been minimized.

Member

dolmen commented Apr 1, 2015

@doyleyoung "CPANPR challenge" is a useless title for a PR. Next time, please use separate PR for each issue solved or each unrelated improvement, and use a meaningful title.

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