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

Add new method "truncate" for RT 62189 #25

Merged
merged 2 commits into from Jan 10, 2017

Conversation

Projects
None yet
2 participants
@openstrike

openstrike commented Oct 3, 2016

Here is an implementation of the proposed "truncate" method discussed in RT 62189. This is not a full replacement for the truncate method of DateTime because it does not handle either of the "week" or "local_week" options. It does cover all the other possible values of the "to" parameter, however.

There's a test script and a documentation update included.

Time::Piece is my module for this month's CPAN PR Challenge - thanks for participating!

@smith153

This comment has been minimized.

Show comment
Hide comment
@smith153

smith153 Oct 7, 2016

Collaborator

Looks like a cool addition! My only concern is the use of Test::Trap. I don't think Test::Trap is included in core? And as such, someone building from source wouldn't be able to run that test (as Test::Trap wouldn't be available). Perhaps one could rewrite it by catching it with eval instead?

Collaborator

smith153 commented Oct 7, 2016

Looks like a cool addition! My only concern is the use of Test::Trap. I don't think Test::Trap is included in core? And as such, someone building from source wouldn't be able to run that test (as Test::Trap wouldn't be available). Perhaps one could rewrite it by catching it with eval instead?

@openstrike

This comment has been minimized.

Show comment
Hide comment
@openstrike

openstrike Oct 7, 2016

No, Test::Trap isn't core. I've used it quite a bit previously and found it to be a simple way to test for exceptions. By couching it in the SKIP block it means that the tests shouldn't fail because of the absence of the module but as you say neither would they be run. It is very easy to replace with plain evals which has been done in the commit b6f2a73 just pushed.

Thanks for reviewing.

openstrike commented Oct 7, 2016

No, Test::Trap isn't core. I've used it quite a bit previously and found it to be a simple way to test for exceptions. By couching it in the SKIP block it means that the tests shouldn't fail because of the absence of the module but as you say neither would they be run. It is very easy to replace with plain evals which has been done in the commit b6f2a73 just pushed.

Thanks for reviewing.

@openstrike

This comment has been minimized.

Show comment
Hide comment
@openstrike

openstrike Jan 6, 2017

Are you happy with the amended PR which removes Test::Trap, @smith153? Are there any further modifications which you would like before merging?

openstrike commented Jan 6, 2017

Are you happy with the amended PR which removes Test::Trap, @smith153? Are there any further modifications which you would like before merging?

@smith153

This comment has been minimized.

Show comment
Hide comment
@smith153

smith153 Jan 6, 2017

Collaborator

Everything looks good to me. I tested it out a month ago or so and didn't see any issues. I should be merging this in next week and pushing out a new release :)

Collaborator

smith153 commented Jan 6, 2017

Everything looks good to me. I tested it out a month ago or so and didn't see any issues. I should be merging this in next week and pushing out a new release :)

@smith153 smith153 merged commit bf27613 into Dual-Life:master Jan 10, 2017

@smith153

This comment has been minimized.

Show comment
Hide comment
@smith153

smith153 Jan 10, 2017

Collaborator

Merged :)

Collaborator

smith153 commented Jan 10, 2017

Merged :)

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