Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
#20921 [Config] Provide shorthand methods for ArrayNodeDefinition::pr…#20923
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
#20921 [Config] Provide shorthand methods for ArrayNodeDefinition::pr…#20923
Uh oh!
There was an error while loading.Please reload this page.
Conversation
skafandri commentedDec 14, 2016
| Q | A |
|---|---|
| Branch? | master |
| Bug fix? | no |
| New feature? | yes |
| BC breaks? | no |
| Deprecations? | no |
| Tests pass? | yes |
| Fixed tickets | #20921 |
| License | MIT |
| return$this->prototype =$this->getNodeBuilder()->node(null,$type)->setParent($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.
could be removed, IMO it doesn't add any additional value
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 think so, I think that the method name is self explanatory. But wouldn't break the code standards check?
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.
Quite the contrary :) We are trying to only add phpdoc when it adds value.
| * Shorthand for prototype('scalar'). | ||
| * | ||
| * @return ScalarNodeDefinition | ||
| */ |
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 about usingscalarPrototype instead (same for the other new methods)?
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.
scalarPrototype sounds better for me. The only motivation behind the current naming is togroup the methods in an IDE auto complete list.
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 understand that. But at least PhpStorm is even with the other names able to suggest all of these methods when typingprototype (if I am not completely mistaken).
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 you're right. That's not a big issue. Most IDE autocomplete are clever enough to show methods containing rather than starting with a string.
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 agree, my IDE does that too, I was only thinking about some friends who are still using IDEs with left prefix only autocomplete support (Netbeans 6). If you think we shouldn't care about this legacy, I am into it.
skafandri commentedDec 15, 2016
I don't understand the generated diff of fabbot.io Is that hook open source? :) |
fabpot commentedDec 18, 2016
👍 |
1 similar comment
xabbuh commentedDec 18, 2016
👍 |
| $this->assertFalse($this->getField($node,'normalizeKeys')); | ||
| } | ||
| publicfunctiontestPrototypeVariable() |
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 about a single test?
skafandri commentedDec 18, 2016
A single test with multiple asserts to cover all the cases. I am fine with that. What do you think guys? |
fabpot commentedDec 19, 2016
Thank you@skafandri. |
…eDefinition::pr… (skafandri)This PR was squashed before being merged into the 3.3-dev branch (closes#20923).Discussion----------#20921 [Config] Provide shorthand methods for ArrayNodeDefinition::pr…| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#20921| License | MITCommits-------22b8490#20921 [Config] Provide shorthand methods for ArrayNodeDefinition::pr…
oleg-andreyev commentedDec 21, 2016
👍 |
This PR was merged into the 2.7 branch.Discussion----------[Config] Improve PHPdoc / IDE autocomplete| Q | A| ------------- | ---| Branch? | 2.7| Bug fix? | yes-ish| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | #... <!-- #-prefixed issue number(s), if any -->| License | MIT| Doc PR | symfony/symfony-docs#... <!--highly recommended for new features-->The missing pieces for fixing the autocomplete in IDE's (tested in `PhpStorm 2016.3.2`).Each method seems to be found now for the framework bundle configuration 😱Actually uses#20923 where needed, but i think we should go with it consistently.Relates to#20290 (comment)Commits-------3f3c6e0 [Config] Improve PHPdoc / IDE autocomplete