nunomaduro / laravel-console-task

✅ Laravel Console Task is a output method for Laravel Console Commands.
MIT License
254 stars 21 forks source link

adds ability to customize error message and fixes to docblocks on tas… #9

Closed surgiie closed 5 years ago

surgiie commented 5 years ago

Let me start by saying this is a great function to add to laravel console commands. This PR adds the ability to customize the error message on failed tasks and adds @param blocks to the docblock in the task macro in LaravelConsoleTaskServiceProvider for both the new error message param and the loading text param that got merged in a previous PR. I think this is useful to let the executing user know why a task failed other than just saying "failed". This way you can customize the error message provide more detail. As an example:


$file = $this->argument('file');

$this->task("Checking file existence.", function () use ($file){

    if(!file_exists($file)){
        return false;
    }

    return true;

}, "The file [$file] doesnt exist");
nunomaduro commented 5 years ago

Thanks so far for your work 👍

surgiie commented 5 years ago

@nunomaduro I can do that. Just curious to why the changes i made to the test class are not right.

Because this would cause a fail for assertion of equal values to writeln call:

$ouputMock
        ....
       ->with("Bar: <error>{$this->customErrorMessage}</error>"); //the expected error

$this->assertFalse(
    $command->task(
        'Bar', 
        function () {
            return false;
        },
        "loading...",
        "I dont match the custom error the output expects" //my defined custom error
    )
);  

Since tests pass when you pass $this->customErrorMessage to both the output's with method and the task method, this means you are successfully defining a custom error?

Just trying to figure out how this does not already test the ability to do so. I just want to make sure i structure a new test(s) correctly. Do you also want a test for both decorated output and not decorated output?

nunomaduro commented 5 years ago

You should remove all the changes so far on the tests file. IMO you Pull request should add 2 new tests. One for decorated output, and another for non decorated output. Where you pass a custom error message in both, and you assert that the message on the error is exacly the same you specified as custom.