Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Console] [DX] Fix command description/help display#26439
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
| $this->writeText(''.$description); | ||
| $this->writeText("\n\n"); | ||
| } | ||
noniagriconomieMar 7, 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.
should as well the$command->getProcessedHelp() return only the$command->getHelp() and not sometimes the$command->getDescription()? so description will be only on top and not at the bottom?
linehttps://github.com/symfony/symfony/pull/26439/files#diff-f21e3938d69281cd0815f081627d9c34R164
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 it should yes, it's weird thatgetProcessedHelp() can return the description., but I'm not sure it's fine to change it now. Instead, could we display theHelp section (callinggetProcessedHelp()) only ifgetHelp() is not null? or only ifgetProcessedHelp() !== getDescription()?
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.
@chalasr ok, I will change the called method here
chalasr commentedMar 7, 2018
@noniagriconomie For other descriptors, I also think they don't need to be updated. In fact, the TextDescriptor seems to be the only one that does not contain the description. By the way, I must say that you're working on a part which is quite hard to update and not very "first-time-contribution friendly" (not contribution friendly at all in fact), and you just do it perfectly for now! |
noniagriconomie commentedMar 7, 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.
@chalasr PR code updated accordingly |
| if ($help =$command->getProcessedHelp()) { | ||
| $help =$command->getProcessedHelp(); | ||
| if ($help !==$command->getDescription()) { |
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$help && $help !== $description
noniagriconomieMar 8, 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.
👍 change is done@chalasr
javiereguiluz 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.
@noniagriconomie I love DX issues, so thanks a lot for contributing this one. And congrats on your first Symfony contribution!
noniagriconomie commentedMar 9, 2018
@javiereguiluz hope this help a lot of sf devs :) |
fabpot commentedMar 10, 2018
@noniagriconomie Thanks for the pull request. I don't think this qualifies as a bug. So, I'm going to merge it into master into 2.7. |
fabpot commentedMar 10, 2018
Thank you@noniagriconomie. |
This PR was merged into the 4.1-dev branch.Discussion----------[Console] Fix tests| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | no <!-- don't forget to update src/**/CHANGELOG.md files -->| BC breaks? | no <!-- seehttps://symfony.com/bc -->| Deprecations? | no <!-- don't forget to update UPGRADE-*.md files -->| Tests pass? | yes <!-- please add some, will be required by reviewers -->| Fixed tickets | N/A <!-- #-prefixed issue number(s), if any -->| License | MIT| Doc PR | N/AFix Console tests after#26439 merge.Commits-------c429fa0 [Console] Fix tests
Hi,
Here is a fix for the issue#26376
I only patch the text descriptor, should I do it for others?
(I do not think so as the main and default one is text)
Thanks