Skip to content

[5.4] use same output style for jobs as migrations - #18773

Merged
taylorotwell merged 4 commits into
laravel:5.4from
Quezler:patch-6
Apr 12, 2017
Merged

[5.4] use same output style for jobs as migrations#18773
taylorotwell merged 4 commits into
laravel:5.4from
Quezler:patch-6

Conversation

@Quezler

@Quezler Quezler commented Apr 11, 2017

Copy link
Copy Markdown
Contributor
  • Adds a message for when a job has started.
  • Make status strings the same width.

See 2328c7a
And #18542

@CedriQ CedriQ left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Slick

@tillkruss

Copy link
Copy Markdown
Contributor

Would you mind adding tests for this?

@Quezler

Quezler commented Apr 11, 2017

Copy link
Copy Markdown
Contributor Author

@tillkruss from what i can find, the current tests only check for the events being fired, nothing related to text output.

@Quezler

Quezler commented Apr 11, 2017

Copy link
Copy Markdown
Contributor Author

And i currently do not see how a test would be written for this/wether it is required at all (the writeOutput function only gets called in the function i changed, from all the files in the entire framework)

break;
case 'failed':
$this->output->writeln('<error>['.Carbon::now()->format('Y-m-d H:i:s').'] Failed:</error> '.$job->resolveName());
break;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Using str_pad() here, might make this more readable.

@Quezler

Quezler commented Apr 11, 2017

Copy link
Copy Markdown
Contributor Author

I'll refractor it in a moment, good point @tillkruss

@Quezler

Quezler commented Apr 11, 2017

Copy link
Copy Markdown
Contributor Author

@tillkruss I have updated the code the best i could, is it more readable now?

@taylorotwell
taylorotwell merged commit d038921 into laravel:5.4 Apr 12, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants