JBlond/php-diff

Unified Cli renderer options incompatible with Main renderer options.

Closed this issue · 8 comments

The UnifiedCli renderer has option cliColor whose only valid value is simple
$this->options['cliColor'] == 'simple'

The Main renderer has the following options:

    /**
     * @var array   Associative array containing the default options available for this renderer and their default
     *              value.
     *              - tabSize           The amount of spaces to replace a tab character with.
     *              - format            The format of the input texts.
     *              - cliColor          Colorized output for cli.
     *              - deleteMarkers     Markers for removed text.
     *              - insertMarkers     Markers for inserted text.
     *              - equalityMarkers   Markers for unchanged and changed lines.
     *              - insertColors      Fore- and background color for inserted text. Only when cloColor = true.
     *              - deleteColors      Fore- and background color for removed text. Only when cloColor = true.
     */
    protected $mainOptions = [
        'tabSize'         => 4,
        'format'          => 'plain',
        'cliColor'        => false,
        'deleteMarkers'   => ['', ''],
        'insertMarkers'   => ['', ''],
        'equalityMarkers' => ['', ''],
        'insertColors'    => ['black', 'green'],
        'deleteColors'    => ['black', 'red'],
    ];

Option cliColor should be of type boolean and when true the output should be colorized using the colors defined at options insertColors and deleteColors.

btw... The comments in the code block above contains typos as well: cloColor instead of cliColor
See

* - insertColors Fore- and background color for inserted text. Only when cloColor = true.
and
* - deleteColors Fore- and background color for removed text. Only when cloColor = true.

Feel very free to change that as you like. We can do a new version number that indicates that it is incompatible.

See my suggestion #61

I wonder if the constructor can be changed from

    public function __construct(array $options = [])
    {
        parent::__construct();
        $this->setOptions($this->subOptions);
        $this->setOptions($options);
    }

to

    public function __construct(array $options = [])
    {
        parent::__construct($options);
    }

since the subOptions are empty in InlineCli

See my comment at #61 (comment)
I think it's the same subject?

subOptions is currently empty, but meant to declare default values for the subRenderer, other than the options of MainRenderer.
(Something like being futureproof)

I still wonder then, why the constructor has the parameter, if setOptions should be used.

Stop wondering, your thoughts are right.
Just wanted to make clear why $this->subOptions exists and all options need to be merged.

The constructor of MainRendererAbstract also uses setOptions.

    public function __construct(array $options = [])
    {
        parent::__construct();
        $this->setOptions($this->subOptions);
        $this->setOptions($options);
    }

could probably be shortened to:

    public function __construct(array $options = [])
    {
        parent::__construct($this->subOptions);
        $this->setOptions($options);
        // Or switch $this->subOptions and $options.
    }

It makes sense to set $options at last. Otherwise you can't override the default subOptions.

Done. Merge #61