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

Support pushing an entire Period instance forward or backwards in time #40

Merged
merged 1 commit into from Sep 15, 2016

Conversation

Projects
None yet
2 participants
@adamnicholson
Contributor

adamnicholson commented Sep 8, 2016

Introduction

Currently, if you wish to advance a Period instance forward or backwards in time, this must be implemented in userland.

An example would be wanting to advance Period('2016-01-01', '2016-07-07') forward 5 days, to result in Period('2016-06-01', '2016-07-12')

Proposal

This PR adds new advance(DateInterval $by) and recede(DateInterval $by) methods to Period, which perform the operation described above.

For example:

$period = new Period('2016-01-01 15:32:12', '2016-01-15 12:00:01');
$moved = $period->advance(new DateInterval('P1D')); // new Period('2016-01-02 15:32:12', '2016-01-16 12:00:01')

Backward Incompatible Changes

There are no BC breaks.

Targeted release version

The next release.

PR Impact

None

Open issues

None

@nyamsprod

This comment has been minimized.

Show comment
Hide comment
@nyamsprod

nyamsprod Sep 8, 2016

Member

@adamnicholson I like the idea but you could improve the implementation by:

  • using directly the constructor
  • using the internal validation method to broaden the usage

For instance your advance method would be better like this

public function advance($interval)
{
    $interval = static::filterDateInterval($interval);

    return new static($this->startDate->add($interval), $this->endDate->add($interval));
}

This way you ensure for instance that someone can do

use League\Period\Period;

$period = Period::createFromDay('today');
$new_period = $period->advance('+1 DAY');
Member

nyamsprod commented Sep 8, 2016

@adamnicholson I like the idea but you could improve the implementation by:

  • using directly the constructor
  • using the internal validation method to broaden the usage

For instance your advance method would be better like this

public function advance($interval)
{
    $interval = static::filterDateInterval($interval);

    return new static($this->startDate->add($interval), $this->endDate->add($interval));
}

This way you ensure for instance that someone can do

use League\Period\Period;

$period = Period::createFromDay('today');
$new_period = $period->advance('+1 DAY');
@adamnicholson

This comment has been minimized.

Show comment
Hide comment
@adamnicholson

adamnicholson Sep 8, 2016

Contributor

@nyamsprod good point, I've updated the PR.

One thought I had was whether this should even be 2 methods, or 1? If it were 1 it would require the user to pass in an inverted DateInterval instance (or use an inverted string) for the recede.

2 methods is a bit more user friendly I thought, but you would get what seems like unusual on the surface (although totally logical) behaviour if you pass in inverted intervals

Eg.

$period = new Period('2016-01-02 15:32:12', '2016-01-16 12:00:01');
$moved = $period->advance('- 1 day'); // new Period('2016-01-01 15:32:12', '2016-01-15 12:00:01')
$period = new Period('2016-01-02 15:32:12', '2016-01-16 12:00:01');
$moved = $period->recede('- 1 day'); // new Period('2016-01-03 15:32:12', '2016-01-17 12:00:01')
Contributor

adamnicholson commented Sep 8, 2016

@nyamsprod good point, I've updated the PR.

One thought I had was whether this should even be 2 methods, or 1? If it were 1 it would require the user to pass in an inverted DateInterval instance (or use an inverted string) for the recede.

2 methods is a bit more user friendly I thought, but you would get what seems like unusual on the surface (although totally logical) behaviour if you pass in inverted intervals

Eg.

$period = new Period('2016-01-02 15:32:12', '2016-01-16 12:00:01');
$moved = $period->advance('- 1 day'); // new Period('2016-01-01 15:32:12', '2016-01-15 12:00:01')
$period = new Period('2016-01-02 15:32:12', '2016-01-16 12:00:01');
$moved = $period->recede('- 1 day'); // new Period('2016-01-03 15:32:12', '2016-01-17 12:00:01')
@nyamsprod

This comment has been minimized.

Show comment
Hide comment
@nyamsprod

nyamsprod Sep 8, 2016

Member

@adamnicholson Good remark. One method also means an easier class to maintain as long as the documentation is clear. So let's make it one method. Let's call it advance and we will just document that in order to get a recede period users will have to use an inverted DateInterval object. and call it a day 👍

Member

nyamsprod commented Sep 8, 2016

@adamnicholson Good remark. One method also means an easier class to maintain as long as the documentation is clear. So let's make it one method. Let's call it advance and we will just document that in order to get a recede period users will have to use an inverted DateInterval object. and call it a day 👍

@adamnicholson

This comment has been minimized.

Show comment
Hide comment
@adamnicholson

adamnicholson Sep 8, 2016

Contributor

Sounds good to me!

Contributor

adamnicholson commented Sep 8, 2016

Sounds good to me!

@nyamsprod

This comment has been minimized.

Show comment
Hide comment
@nyamsprod

nyamsprod Sep 8, 2016

Member

@adamnicholson yep just squash all your commits into one for clean history reason and I'll merge your PR for a 3.3.0 release probably next week!

Member

nyamsprod commented Sep 8, 2016

@adamnicholson yep just squash all your commits into one for clean history reason and I'll merge your PR for a 3.3.0 release probably next week!

@nyamsprod

This comment has been minimized.

Show comment
Hide comment
@nyamsprod

nyamsprod Sep 8, 2016

Member

@adamnicholson you should also make a PR on the gh-pages to update the documentation by adding the relevant info

Member

nyamsprod commented Sep 8, 2016

@adamnicholson you should also make a PR on the gh-pages to update the documentation by adding the relevant info

@nyamsprod

This comment has been minimized.

Show comment
Hide comment
@nyamsprod

nyamsprod Sep 15, 2016

Member

@adamnicholson after a week-end to think about it I think the method should be called move

public Period::move($interval): Period

This way depending on the interval submitted we move forward or backward both datepoints and it will make more sense IMHO.

If you make this change and squash your intermediary commits I'll merge your PR ASAP

Member

nyamsprod commented Sep 15, 2016

@adamnicholson after a week-end to think about it I think the method should be called move

public Period::move($interval): Period

This way depending on the interval submitted we move forward or backward both datepoints and it will make more sense IMHO.

If you make this change and squash your intermediary commits I'll merge your PR ASAP

@adamnicholson

This comment has been minimized.

Show comment
Hide comment
@adamnicholson

adamnicholson Sep 15, 2016

Contributor

Good shout. Done that now.

Contributor

adamnicholson commented Sep 15, 2016

Good shout. Done that now.

@nyamsprod nyamsprod merged commit 087faef into thephpleague:master Sep 15, 2016

1 check passed

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

This comment has been minimized.

Show comment
Hide comment
@nyamsprod

nyamsprod Sep 19, 2016

Member

@adamnicholson version 3.3.0 is release with the move method 🎱

Member

nyamsprod commented Sep 19, 2016

@adamnicholson version 3.3.0 is release with the move method 🎱

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