yohang / CalendR

The missing PHP 5.3+ calendar management library.
http:/yohan.giarel.li/CalendR
MIT License
465 stars 65 forks source link

allow use of alternate Factory that implements FactoryInterface. #27

Closed ovgray closed 11 years ago

ovgray commented 11 years ago

This PR modifies the present alternate-periods branch to allow use of alternate Factory that implements a new FactoryInterface, rather than having to extend CalendR\Period\Factory as it would with the current. It also moves the alternate period creation logic used by Calendar.php back to Calendar.php from Period/Factory.php, to simplify the Factory Interface. FactoryTest is modified to reflect these changes.

I have not changed the phpdoc "@return" statements in Factory and elsewhere that assume that FactoryInterface methods return objects that extend CalendR\Period\ objects. I note that the AlternatePeriodsTest only verifies that the code will handle alternate period objects that extend those objects.

It might be useful to permit alternate period objects that implement PeriodInterface (or, better, an Interface specific to Year, Month, Week, Day and Range objects) without having to extend the existing objects.

If AlternatePeriodsTest reflects what you consider to be the required behavior of any Year, Month, Week, Day or Range object, then I could work out the interfaces from that.

yohang commented 11 years ago

Seems good, can you run a CS fix on your PR ? http://cs.sensiolabs.org

ovgray commented 11 years ago

sorry about the multiple commits - would you like me to close this PR and submit a new PR with all this in one commit?

yohang commented 11 years ago

It seems good, could you just squash your commits and put docblocks on FactoryInterface methods and replace Factory class docblocks by

/**
 * {@inheritDoc}
 */
ovgray commented 11 years ago

I have added docblocks to FactoryInterface and squashed the commits.

The methods that create and return Periods are marked "@return \CalendR\Period\PeriodInterface" so that alternate periods need not extend the existing ones. The remaining question is whether Interfaces should be specified for the Periods.

yohang commented 11 years ago

I have thought to add Interfaces for periods, but there is almost no difference between period methods, so it wouldn't be really useful.