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 diff text visibility for console reporter #500

Merged
merged 6 commits into from Dec 11, 2015
Merged

Conversation

@thephilip
Copy link
Contributor

thephilip commented Dec 7, 2015

Modified internals.color to accept an array of color codes and added new color codes to the internals.colors method as well.

See: #493

thephilip added 3 commits Dec 7, 2015
CI tool was complaining about initialized variables being unused.  Removed them.
Tested with xterm and cmd/cygwin now.  Untested with Terminal.app (OSX).
@geek geek changed the title fix diff text visibility fix diff text visibility for console reporter Dec 11, 2015
@@ -341,6 +341,14 @@ internals.color = function (name, code, enabled) {
};
}

if (enabled && Array.isArray(code)) {
const color = '\u001b[' + code[0] + ';' + code[1] + 'm';

This comment has been minimized.

Copy link
@geek

geek Dec 11, 2015

Member

doesn't look safe, you will want to check the length... might also want to do this condition first and else if enabled do the one on line 336

This comment has been minimized.

Copy link
@thephilip

thephilip Dec 11, 2015

Author Contributor

I will do that. Thank you for the suggestions.

@thephilip

This comment has been minimized.

Copy link
Contributor Author

thephilip commented Dec 11, 2015

@geek: Besides not remembering code style requirements, I think I got the majority of what you asked for done, albeit I should have done most of that in the first place. I am considering adding the array length check in internals.colors instead of internals.color, but I'm unsure how it should be handled. Should I output an error message/warning simply limit the array length, both, or something else?

@geek geek added the feature label Dec 11, 2015
@geek geek added this to the 8.0.1 milestone Dec 11, 2015
@geek geek added bug and removed feature labels Dec 11, 2015
@geek geek self-assigned this Dec 11, 2015
geek added a commit that referenced this pull request Dec 11, 2015
fix diff text visibility for console reporter
@geek geek merged commit ee2e2be into hapijs:master Dec 11, 2015
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.