-
Notifications
You must be signed in to change notification settings - Fork 736
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
minor changes in Query class #733
Conversation
*/ | ||
public function setRescore($rescore) | ||
public function setRescore(AbstractRescore $rescore) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You realize that by adding a new typehint to class that somebody might be extending is a BC break? Adding typehints is a good thing, but then the release process should take that into account (=change like this should not appear in patch version release and ideally not even in feature version release).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@webdevsHub I suggest you mention this in the changes.txt as a BC break.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just thought it would be better then getting a Fatal Error on
$rescore->toArray() line.
What do you suggest to do to take the release process into account?
Am 14.12.2014 18:36 schrieb "Filip Procházka" [email protected]:
In lib/Elastica/Query.php
#733 (diff):*/
- public function setRescore($rescore)
- public function setRescore(AbstractRescore $rescore)
You realize that by adding a new typehint to class that somebody might be
extending is a BC break? Adding typehints is a good thing, but then the
release process should take that into account (=change like this should not
appear in patch version release and ideally not even in feature version
release).—
Reply to this email directly or view it on GitHub
https://github.com/ruflin/Elastica/pull/733/files#r21797723.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can always do "manual check"
if (!$rescore instanceof AbstractRescore) {
throw new \LogicException(sprintf('Instance of AbstractRescore expected, but %s was given.', get_type($rescore)));
}
return $this->setParam('rescore', $rescore->toArray());
About the release process, it doesn't matter if change like this is in master branch, but it does matter if it's in some release-branch and if it it's tagged.
@webdevsHub We could go a step by step approach. With the option proposed by @fprochazka we "break" BC but it throws a proper exception. For me both options are fine, important is that such breaks are mentioned in the release log (changes.txt). Can you update the changes.txt file? |
okay, I removed that typehint. The important one was the |
Merged. Thx. |
small improvements for new documentation in ruflin/Elastica.io#27