Skip to content

Php diff 60 #61

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.

Sign up for GitHub

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

Merged
merged 9 commits into from
Oct 8, 2020
Merged

Php diff 60 #61

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
df270f5
correct typos
JBlond Oct 6, 2020
38864db
change cli colors option from simple to true
JBlond Oct 6, 2020
fbccaf4
check type of option
JBlond Oct 6, 2020
98781fd
check only if option is set
JBlond Oct 7, 2020
0e3fd21
merge options instead of set them directly
JBlond Oct 7, 2020
24c8bb9
Simplify options merge
JBlond Oct 7, 2020
bd6297c
Merge branch 'master' into php-dif-60
JBlond Oct 7, 2020
276d84d
Simplify code
JBlond Oct 7, 2020
8098c71
if option is true, not only if it is set
JBlond Oct 7, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ $options = [
'ignoreWhitespace' => true,
'ignoreCase' => true,
'context' => 2,
'cliColor' => 'simple' // for cli output
'cliColor' => true // for cli output
];

// Initialize the diff class.
Expand Down
2 changes: 1 addition & 1 deletion example/cli.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@
$renderer = new UnifiedCli(
// Define renderer options.
[
'cliColor' => 'simple',
'cliColor' => true,
]
);

Expand Down
4 changes: 2 additions & 2 deletions lib/jblond/Diff/Renderer/MainRendererAbstract.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,8 @@ abstract class MainRendererAbstract
* - 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.
* - insertColors Fore- and background color for inserted text. Only when cliColor = true.
* - deleteColors Fore- and background color for removed text. Only when cliColor = true.
*/
protected $mainOptions = [
'tabSize' => 4,
Expand Down
7 changes: 2 additions & 5 deletions lib/jblond/Diff/Renderer/Text/UnifiedCli.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,10 +49,7 @@ public function render(): string
if (!isset($this->options['cliColor'])) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Owner Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

return $this->output();
}
if (isset($this->options['cliColor']) && $this->options['cliColor'] == 'simple') {
return $this->output();
}
throw new InvalidArgumentException('Invalid cliColor option');
return $this->output();
}


Expand All @@ -63,7 +60,7 @@ public function render(): string
*/
private function colorizeString($string, $color = ''): string
{
if (isset($this->options['cliColor']) && $this->options['cliColor'] == 'simple') {
if (isset($this->options['cliColor'])) {
return $this->colors->getColoredString($string, $color);
}
return $string;
Expand Down