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

Unified Cli renderer options incompatible with Main renderer options. #60

Closed
DigiLive opened this issue Oct 6, 2020 · 8 comments
Closed

Comments

@DigiLive
Copy link
Collaborator

DigiLive commented Oct 6, 2020

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.

@JBlond
Copy link
Owner

JBlond commented Oct 6, 2020

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

@JBlond
Copy link
Owner

JBlond commented Oct 6, 2020

See my suggestion #61

@JBlond
Copy link
Owner

JBlond commented Oct 7, 2020

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

@DigiLive
Copy link
Collaborator Author

DigiLive commented Oct 7, 2020

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)

@JBlond
Copy link
Owner

JBlond commented Oct 7, 2020

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

@DigiLive
Copy link
Collaborator Author

DigiLive commented Oct 7, 2020

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.
    }

@JBlond
Copy link
Owner

JBlond commented Oct 7, 2020

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

@JBlond
Copy link
Owner

JBlond commented Oct 8, 2020

Done. Merge #61

@JBlond JBlond closed this as completed Oct 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants