Skip to content
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

Fix #73 and support immutable dates #75

Merged
merged 7 commits into from
Dec 7, 2018

Conversation

kylekatarnls
Copy link
Collaborator

.gitignore Outdated
@@ -2,3 +2,4 @@ build
composer.lock
docs
vendor
.idea
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this, it should be in a global gitignore

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cleaned, it will disappear if you re-open the PR.

@@ -150,13 +151,17 @@ public function isClosed(): bool
return $this->isClosedAt(new DateTime());
}

public function nextOpen(DateTimeInterface $dateTime): DateTime
public function nextOpen(DateTimeInterface $dateTime): DateTimeInterface
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately changing the return type is a breaking change.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nop because your PHP requirement: php: ^7.0 implies that any DateTime object implements DateTimeInterface, so unless using reflexion, there is no break, but with reflexion, anything is breaking change.

@freekmurze
Copy link
Member

Closing this as changing the return type is a breaking change. We'll revisit this when creating a new major version of the package.

@freekmurze freekmurze closed this Nov 5, 2018
@kylekatarnls
Copy link
Collaborator Author

kylekatarnls commented Nov 5, 2018

@freekmurze Nop because your PHP requirement: php: ^7.0 implies that any DateTime object implements DateTimeInterface, so unless using reflexion, there is no break, but with reflexion, anything is a breaking change.

So you should not consider it as a breaking change and could safely implement this in the current major version.

@kylekatarnls
Copy link
Collaborator Author

Then you still have the missing reset($timeRange);, this bug should be fixed in the current version.

@freekmurze
Copy link
Member

freekmurze commented Nov 5, 2018

It is a breaking change.

Consider this code:

class A
{
    public function date(): DateTime
    {
        return new DateTime();
    }
}

class B extends A
{
    public function date(): DateTime
    {
        return new DateTime();
    }
}

(new B)->date();

If you change the return type of class A to DateTimeInterface you'll get an error:

Declaration of B::date(): DateTime must be compatible with A::date(): DateTimeInterface

@kylekatarnls
Copy link
Collaborator Author

kylekatarnls commented Nov 5, 2018

Still, this could have been done differently, simply closing this PR rather than asking for modifications (or take it partially to get the bug fixes, or tag it to be merged in next major version) seems very counter-productive to me. I recommend you a more encouraging contributing process for your PRs such as discuss first, propose alternatives, then decide when every participant finished to consider possible solutions.

@freekmurze
Copy link
Member

Thank you for your work around this. I'm sorry that my comments seems counter-productive to you.

Feel free to either rework this PR (in that case I'll reopen it when you push changes) or submit a new PR altogether.

@kylekatarnls kylekatarnls mentioned this pull request Nov 21, 2018
@kylekatarnls kylekatarnls added this to the v2 milestone Nov 29, 2018
@kylekatarnls kylekatarnls reopened this Nov 29, 2018
@kylekatarnls kylekatarnls self-assigned this Dec 4, 2018
@kylekatarnls kylekatarnls changed the base branch from master to version-2.0 December 7, 2018 13:23
@kylekatarnls kylekatarnls mentioned this pull request Dec 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants