Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
Distinguish between first and subsequent progress bar displays#19134
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
With the changes made bysymfony#19134, the `\Symfony\Component\Console\Tests\Helper\ProgressBarTest::testMultipleStart()` test fails.This patch fixes the test forsymfony#19134.
| */ | ||
| privatefunctionoverwrite($message) | ||
| { | ||
| static$firstTime =true; |
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 better as a property, isn't it?
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.
Having a private property is fine.
The pain is that this is going to be a test every time the progress bar is updated.
Somewhere there has to be a switch between 1st and not 1st, so I suspect there is no way to not have the test.
nicolas-grekas commentedJun 21, 2016
Tests need to pass of course, and new tests added to cover the behavior. |
rquadling commentedJun 21, 2016
stof commentedJun 21, 2016
@rquadling you should get rid of only one of them, and update the other PR to contain all changes |
fabpot commentedJun 21, 2016
I've closed#19135. You can do the change directly in this PR now. No need to create yet another one. |
a535c71 to1ca75adCompare| /** | ||
| * @group time-sensitive | ||
| * @group pb |
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.
why have you added a new group here?
fabpot commentedJun 23, 2016
Thank you@rquadling. |
…lays (rquadling)This PR was merged into the 2.7 branch.Discussion----------Distinguish between first and subsequent progress bar displays| Q | A| ------------- | ---| Branch? | 2.7| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#19133| License | MIT| Doc PR | reference to the documentation PR, if anyFixes#19133When a progress bar is first displayed, if it is multi-line, previously output lines are erased, depending upon the number of lines in the progress bar.This patch fixes that be distinguishing between the first display (no erasing of previous output) and subsequent displays of the progress bar.Commits-------3871e1a Differentiate between the first time a progress bar is displayed and subsequent times
rquadling commentedJun 23, 2016
Thank you. |
| privatefunctionisFirstRun() | ||
| { | ||
| return$this->firstRun; |
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.
imo this method as well as the one below should have been inlined
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.
Of course!#19163
Uh oh!
There was an error while loading.Please reload this page.
Fixes#19133
When a progress bar is first displayed, if it is multi-line, previously output lines are erased, depending upon the number of lines in the progress bar.
This patch fixes that be distinguishing between the first display (no erasing of previous output) and subsequent displays of the progress bar.