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

Fix: do not display graphiz when argument is set false #683

Merged
merged 2 commits into from
Oct 16, 2019
Merged

Fix: do not display graphiz when argument is set false #683

merged 2 commits into from
Oct 16, 2019

Conversation

jerowork
Copy link
Contributor

@jerowork jerowork commented Sep 28, 2019

Q A
Branch master for features and deprecations
Bug fix? yes
New feature? no
BC breaks? yes
Deprecations? no
Documented? yes
Fixed tickets none

Description

By default, addOptionalArgument in ProcessArgumentsCollection discards an argument when the value evaluates to false.
This results in the absence of the option in the final process command.

In many cases, the absence of an option means false, but in the case of Deptrac's --formatter-graphviz-display it means true.

The use of addOptionalArgument will never allow false, so unsetting this option in Deptrac is impossible.
In Deptrac.php all options are always present, so adding it just by default with add will fix this.

Edit:
Because the default value for this option --formatter-graphviz-display is true at Deptrac itself, the default here should be true too, to align. This means a small BC because the default is different.

Sources

Deptrac: option formatter_graphviz_display is set true by default:

GrumPHP: option formatter_graphviz_display is set false by default:

By default, addOptionalArgument discards an argument when the value
evaluates to false.
This results in the absence of the option in the final process command.

In many cases, the absence of an option means false, but in the case of
Deptrac's --formatter-graphviz-display it means true.

The use of addOptionalArgument will never allow false, so unsetting this
option in Deptrac is impossible.
@veewee
Copy link
Contributor

veewee commented Oct 10, 2019

Thanks for the detailed PR @jerowork!
The code looks good.

However : I'dd prefer to have the defaults similar to the once of the source tool as well.
Would this be something you could change + document as well?

@veewee veewee added this to the 0.16.2 milestone Oct 10, 2019
@jerowork
Copy link
Contributor Author

@veewee I'd thought of it, but that would change the current default behavior of this parameter when not set in the config.

I am in favour of having the same default values as the source tool, so I will update this PR to align with Deptrac's defaults 👍

In order to have the same default value as Deptrac itself, the default
for the option --formatter_graphviz_display is set to true.
@veewee
Copy link
Contributor

veewee commented Oct 16, 2019

Thanks for your work! Looks good!

@veewee veewee merged commit 5589fac into phpro:master Oct 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants