Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Workflow] Added DefinitionBuilder::setMetadataStore().#27190
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
04d528b to675e6fdCompare| * @param string[] $places | ||
| * @param Transition[] $transitions | ||
| * @param string|null $initialPlace | ||
| * @param MetadataStoreInterface|null $metadataStore |
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.
We prefer doing partial docblocks, keeping them only when they provide something over the signature:
instead of adding this, I'd suggest to remove the one for initialPlace
| } | ||
| /** | ||
| * @param Transition $transition |
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.
should be removed (see previous comment)
| } | ||
| /** | ||
| * @param MetadataStoreInterface $metadataStore |
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.
should be removed
vudaltsov commentedMay 7, 2018
@nicolas-grekas , thank you for explanations, done. |
lyrixx left a comment
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.
Thanks for this PR. I left a tiny comment ;)
vudaltsov commentedMay 8, 2018
@lyrixx , can't see it... |
| } | ||
| /** | ||
| * @return $this |
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 would use a type hint instead of a php doc
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.
What type hint?: self?
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.
yes ;)
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.
SupposeNewDefinitionBuilder extends our non-finalDefinitionBuilder. Then return type of$newDefinitionBuilder->setMetadataStore() will beDefinitionBuilder, notNewDefinitionBuilder.
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.
Yes indeed. And it's not an issue ;)
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.
@lyrixx But method autocompletion in IDE for methods from the inherited class will stop after a call to a method withself as return type.
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.
Typehinting fluent pattern with: self is okay in a fluent interface because you are not supposed to add any new public methods in it's implementations. And probably in a final class for the same reason.
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.
found some relevant occurrences in symfony code:
but I still don't agree it is correct...
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.
With: self it is actually correct to do this:
<?phpclass A{publicfunctionfoo():self {return$this; }}class Bextends A{publicfunctionfoo():A {returnnewA(); }}
It does not contradict the contract.
While@return $this indicates that the returned value should be the same instance.
nicolas-grekasMay 9, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
Makes sense to keep it as this provides more than just the return type.
In the code base, we use both return static and $this, dunno if there is a difference thought here.
| } | ||
| /** | ||
| * @return $this |
nicolas-grekasMay 9, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
Makes sense to keep it as this provides more than just the return type.
In the code base, we use both return static and $this, dunno if there is a difference thought here.
62b352c to2882f8dComparefabpot commentedMay 11, 2018
Thank you@vudaltsov. |
…(vudaltsov)This PR was squashed before being merged into the 4.1 branch (closes#27190).Discussion----------[Workflow] Added DefinitionBuilder::setMetadataStore().| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets || License | MIT| Doc PR |This PR complements#26092.Commits-------2882f8d [Workflow] Added DefinitionBuilder::setMetadataStore().
This PR complements#26092.