Skip to content
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

Write task progress to error output #399

Merged
merged 1 commit into from
Nov 27, 2017
Merged

Write task progress to error output #399

merged 1 commit into from
Nov 27, 2017

Conversation

jyggen
Copy link
Contributor

@jyggen jyggen commented Aug 31, 2017

Q A
Branch master
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Documented? -
Fixed tickets -

Currently things that uses $this->output (like and ) writes to stdout while everything that utilizes ProgressBar usually writes to stderr. This PR simply replicates how ProgressBar handles output internally to make output a bit more consistent.

Copy link
Contributor

@veewee veewee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure if we should output to stderr. Maybe it's better to output everything to stdout instead and output the actual errors to stderr? What do you think?

@@ -34,6 +35,10 @@ class ProgressSubscriber implements EventSubscriberInterface
*/
public function __construct(OutputInterface $output, ProgressBar $progressBar)
{
if ($output instanceof ConsoleOutputInterface) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also agree with @veewee

@veewee
Copy link
Contributor

veewee commented Oct 27, 2017

Hi @jyggen,

Do you agree with the comments above?

@jyggen
Copy link
Contributor Author

jyggen commented Nov 27, 2017

Sorry for my inactivity here, but hopefully the PR looks better now!

Whether it's better to output everything to stdout instead of stderr is something worth discussing indeed, but the point of this PR is simply to normalize the usage. I've seen build systems where stdout and stderr are sent to different places/files, and it looks really weird when the task names end up in one place and ✔ and ✘ in another :)

@Landerstraeten Landerstraeten added this to the Version 0.13.0 milestone Nov 27, 2017
@Landerstraeten Landerstraeten merged commit f01be79 into phpro:master Nov 27, 2017
@Landerstraeten
Copy link
Contributor

Thank you @jyggen!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants