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

Update minimum perl version, put it to META.yml #38

Merged
merged 2 commits into from Aug 16, 2018

Conversation

Projects
None yet
2 participants
@kyzn
Contributor

kyzn commented Aug 16, 2018

こんにちは!It was a pleasure to meet you in Glasgow!

Thanks for maintaining Text-CSV. I've made some work on minimum perl version for my monthly CPAN Pull Request Challenge assignment.

I've found at Kwalitee that this module doesn't specify a minimum perl version at META.YML file. This can be seen here: https://cpants.cpanauthors.org/release/ISHIGAKI/Text-CSV-1.96

I've used perlver command to find what is the minimum perl version. Some test files have 5.8.0 due rule _open_scalar. This is the highest throughout the repository.

Another issue is that CSV_PP.pm explicitly specifies 5.5.0, whereas it actually needs 5.6.0 due rule _regex. First commit in this PR updates both CSV_PP.pm and Makefile.pl to 5.8.0.

Second commit specifies 5.8 at Makefile.PL, which will make it appear on META.yml file. This fixes the kwalitee issue.

Please let me know if you'd rather keep it at 5.6, and use an older syntax in those test files. That should not be too difficult to implement.

Cheers!

@charsbar

This comment has been minimized.

Show comment
Hide comment
@charsbar

charsbar Aug 16, 2018

Collaborator

こんにちは。Thanks for your PR. The tests have lots of $] checks to skip under older versions of Perl. These tests are taken almost verbosely from Text::CSV_XS, and Tux assured me that all the tests passed under Perl 5.6.1 (at least for Text::CSV_XS). Unfortutenaly I don't have 5.6.1 at hand, but I confirmed they passed for the latest Text::CSV under Perl 5.6.2 as well.

Could you change the minimum version to 5.006001?

Collaborator

charsbar commented Aug 16, 2018

こんにちは。Thanks for your PR. The tests have lots of $] checks to skip under older versions of Perl. These tests are taken almost verbosely from Text::CSV_XS, and Tux assured me that all the tests passed under Perl 5.6.1 (at least for Text::CSV_XS). Unfortutenaly I don't have 5.6.1 at hand, but I confirmed they passed for the latest Text::CSV under Perl 5.6.2 as well.

Could you change the minimum version to 5.006001?

@kyzn

This comment has been minimized.

Show comment
Hide comment
@kyzn

kyzn Aug 16, 2018

Contributor

Sure thing! Just a moment.

Contributor

kyzn commented Aug 16, 2018

Sure thing! Just a moment.

kyzn added some commits Aug 16, 2018

Update minimum perl version to 5.6.1
Perl::MinimumVersion showed that CSV_PP.pm needs 5.6 for
the _regex rule, but 5.5 was specified instead.

Some test files may need 5.8 for _open_scalar, but this is currently
not an issue as most tests check for perl version.
Specify minimum perl = 5.6.1 at Makefile.PL
This will show up as requires.perl in META.yml
@kyzn

This comment has been minimized.

Show comment
Hide comment
@kyzn

kyzn Aug 16, 2018

Contributor

I updated my commits so that it's 5.6.1 now. Thanks!

Contributor

kyzn commented Aug 16, 2018

I updated my commits so that it's 5.6.1 now. Thanks!

@charsbar charsbar merged commit 7a3db0e into makamaka:master Aug 16, 2018

@charsbar

This comment has been minimized.

Show comment
Hide comment
@charsbar

charsbar Aug 16, 2018

Collaborator

Thanks!

Collaborator

charsbar commented Aug 16, 2018

Thanks!

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