Skip to content

Remove --color=true#157

Closed
paladox wants to merge 3 commits into
Marak:masterfrom
paladox:patch-1
Closed

Remove --color=true#157
paladox wants to merge 3 commits into
Marak:masterfrom
paladox:patch-1

Conversation

@paladox
Copy link
Copy Markdown
Contributor

@paladox paladox commented Apr 20, 2016

Reason since you can either use --color=false to disable colors or use colors which should be on by default without needing to use a config.

1.x release broke grunt see gruntjs/grunt-legacy-log#16

So instead if it detect that --color=false or --no-color are not be using then it will switch colors on by default like it use too.

Reason since you can either use --color=false to disable colors or use colors which should be on by default without needing to use a config.

1.x release broke grunt see gruntjs/grunt-legacy-log#16

So instead if it detect that --color=false or --no-color are not be using then it will switch colors on by default like it use too.
}

return false;
})(); No newline at end of file
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Please avoid unrelated changes like this.

Copy link
Copy Markdown
Contributor Author

@paladox paladox Apr 20, 2016

Choose a reason for hiding this comment

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

Hi sorry, GitHub does it, I tried to create a new pull to see if that would work but it still does the bottom.

argv.indexOf('--color=true') !== -1 ||
argv.indexOf('--color=always') !== -1) {
if (!argv.indexOf('--no-color') !== -1 ||
!argv.indexOf('--color=false') !== -1) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This is wrong for multiple reasons.

  1. The || is an or, which means this will always enable color when either the first or the second argument is missing. This is always the case, except when you specify both arguments, which does not make much sense. I assume this was meant to be an &&.
  2. With an && this is the exact inverse of the if above. Which means all the code below this second if can never be reached.

Look at the code below. The issue you are experiencing may be hidden there.

I suggest to close this patch.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hi this was just a copy of the above code except from I added ! so if --no-color is used color is disabled if it is not used then color is enabled by default.

Copy link
Copy Markdown

@thiemowmde thiemowmde Apr 20, 2016

Choose a reason for hiding this comment

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

This is not how you invert a logical condition. I already described it in detail above.

@paladox paladox closed this Apr 20, 2016
@paladox
Copy link
Copy Markdown
Contributor Author

paladox commented Apr 20, 2016

In favour of #154 which fixed it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants