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

Strict checking for conversion of strings to DateTime objects #5

Merged
merged 4 commits into from Sep 5, 2018

Conversation

Projects
None yet
2 participants
@HaraldJoerg
Contributor

HaraldJoerg commented Sep 4, 2018

This PR compensates for changes in DateTime::Format::Strptime. That module now allows minutes to be omitted from the timezone, and per default ignores trailing garbage. Unfortunately, this makes a test with an invalid time stamp run without throwing up - which it should.

As of DateTime::Format::Strptime version 1.71, there's a "strict" parameter which does no longer allow trailing garbage. This is now set, the version requirement added to the code.

I have not yet committed stuff for the Changes file because I doubt this can be automatically merged with the other PR coming soon.

@HaraldJoerg

This comment has been minimized.

Show comment
Hide comment
@HaraldJoerg

HaraldJoerg Sep 4, 2018

Contributor

I just noted that the CI tests croak because dzil complains about a duplicate LICENSE file. You have one in your code base, and dzil tries to create its own. Both license files are fundamentally different. I guess dist.ini would need some tuning, but need your advice in which direction.

I can reproduce that:

[DZ] attempt to add LICENSE multiple times; added by: filename set by GatherDir (Dist::Zilla::Plugin::GatherDir line 225); encoded_content added by @Starter/GatherDir (Dist::Zilla::Plugin::GatherDir line 226); content added by @Starter/License (Dist::Zilla::Plugin::License line 37)
aborting; duplicate files would be produced at /home/haj/perl5/perlbrew/perls/perl-5.28.0/lib/site_perl/5.28.0/Dist/Zilla/App/Command/build.pm line 107.

Contributor

HaraldJoerg commented Sep 4, 2018

I just noted that the CI tests croak because dzil complains about a duplicate LICENSE file. You have one in your code base, and dzil tries to create its own. Both license files are fundamentally different. I guess dist.ini would need some tuning, but need your advice in which direction.

I can reproduce that:

[DZ] attempt to add LICENSE multiple times; added by: filename set by GatherDir (Dist::Zilla::Plugin::GatherDir line 225); encoded_content added by @Starter/GatherDir (Dist::Zilla::Plugin::GatherDir line 226); content added by @Starter/License (Dist::Zilla::Plugin::License line 37)
aborting; duplicate files would be produced at /home/haj/perl5/perlbrew/perls/perl-5.28.0/lib/site_perl/5.28.0/Dist/Zilla/App/Command/build.pm line 107.

@heytrav

This comment has been minimized.

Show comment
Hide comment
@heytrav

heytrav Sep 4, 2018

Owner

you can get rid of the license file in code if that helps. I'm happy to use the one generated by dzil. I remember running into this issue before on my ParseUtil::Domain module as well, once up on a time.

Owner

heytrav commented Sep 4, 2018

you can get rid of the license file in code if that helps. I'm happy to use the one generated by dzil. I remember running into this issue before on my ParseUtil::Domain module as well, once up on a time.

@HaraldJoerg

This comment has been minimized.

Show comment
Hide comment
@HaraldJoerg

HaraldJoerg Sep 5, 2018

Contributor

I have removed the LICENSE file in the repository, but also added a COPYRIGHT AND LICENSE section in your main module to satisfy CPANTS kwalitee check. I've also added to the Changes file.

With these changes in place, the PR #6 is no longer needed. I did not do the same changes for that PR.

Looks as if the distribution now passes the CI test. Cheers!

Contributor

HaraldJoerg commented Sep 5, 2018

I have removed the LICENSE file in the repository, but also added a COPYRIGHT AND LICENSE section in your main module to satisfy CPANTS kwalitee check. I've also added to the Changes file.

With these changes in place, the PR #6 is no longer needed. I did not do the same changes for that PR.

Looks as if the distribution now passes the CI test. Cheers!

@heytrav

This comment has been minimized.

Show comment
Hide comment
@heytrav

heytrav Sep 5, 2018

Owner

Ok this looks good to me. Happy to merge

Owner

heytrav commented Sep 5, 2018

Ok this looks good to me. Happy to merge

@heytrav heytrav merged commit dfc8edc into heytrav:master Sep 5, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment