Php diff 60

[JBlond]

No description provided.

[DigiLive]

I think if ($this->options['cliColor']) { is sufficient.

The variable should be set already by MainRendererAbstract and acting on any truthy value is ok.

[JBlond]

I changed that

[DigiLive]

I think if ($this->options['cliColor']) { is sufficient.

The variable should be set already by MainRendererAbstract and acting on any truthy value is ok.

[JBlond]

I changed that.

[DigiLive]

I've made some inline comments.

[DigiLive]

$this->options['cliColor'] should be set by MainRendererAbstract.
Therefor this line will always evaluate to true.

Please also check remaining code which uses this variable.

[JBlond]

It is not always true. I check that via the example/cli.php script.

[DigiLive]

That's why I'm saying 'should' while it's not. 😉
When extending MainRendererAbstract, the options for a sub renderer are supposed to be a merge of:

• MainRendererAbstract::$mainOptions (Default renderer options)
• SubRenderer::$subOptions (Default child renderer options)
• The array parameter of SubRenderer::__construct() (custom options).

SubRenderer::__construct() should look something like:

    public function __construct(array $options = [])
    {
        parent::__construct();
        $this->setOptions($this->subOptions); // Override/Add options MainRenderer with defaults of SubRenderer.
        $this->setOptions($options); // Override/Add options MainRenderer with customs.
    }

Method setOptions, inherited from MainRendererAbstract, will take care of the merges.

Currently UnifiedCli::__construct() is different:

    public function __construct(array $options = [])
    {
        parent::__construct($options);
        $this->colors = new CliColors();
        $this->options = $options;
    }

You can look at InlineCli.php as an example on how to apply options.
Also it's described at https://github.com/JBlond/php-diff/wiki/b)-Parameters-and-Options#parameters-1
and https://github.com/JBlond/php-diff/wiki/b)-Parameters-and-Options#text-unified-cli-renderer

[DigiLive]

Please also check remaining code which uses $this->options.
Line 57-59 can be removed?
Line 71 always evaluates to true? Remove isset function?

[JBlond]

Please also check remaining code which uses $this->options.

I did.

Line 57-59 can be removed?

Yes

Line 71 always evaluates to true? Remove isset function?

Nope, in doubt the colorizeString function returns the string as is.

[DigiLive]

Nope, in doubt the colorizeString function returns the string as is.

I'm not sure what 'In doubt' means.

Currently:

• $string is colorized when the option is set (Which what MainRenderAbstract will do), no matter the value is a truthy or falsy.
• $string is returned as is when the option isn't set (But it will be set by MainRendererAbstract::__construct() calling MainRendererAbstract::setOptions()).

Expected:

• $string is colorized when the option is set and the value is a truthy.
• $string is returned as is when the option isn't set or when the value is a falsy.

Having if ($this->options['cliColor']) {...
The way I see it, $this->options['cliColor'] is either set (by the MainRenderer) and its value is a truthy or falsy, or it isn't set which evaluates to a falsy also. (https://3v4l.org/vGsYB)

[DigiLive]

I think it's all ok now.
Maybe we should user ask/warn vendeeglobe there's a PR merge comming up and ask for his feedback.

[JBlond]

@vendeeglobe please give feedback to this PR.

[vendeeglobe]

Looks good to me. I'll pull and test it after it's merged into master.