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

update default colors to match the node client #32

Merged
merged 3 commits into from Jan 30, 2017
Merged

Conversation

waldyrious
Copy link
Member

At the time of writing, the latest version of the node client has the following configuration:
https://github.com/tldr-pages/tldr-node-client/blob/v1.6.0/config.json

At the time of writing, the latest version of the node client has the following configuration:
https://github.com/tldr-pages/tldr-node-client/blob/v1.6.0/config.json
@waldyrious
Copy link
Member Author

waldyrious commented Oct 1, 2016

@waldyrious
Copy link
Member Author

Any help addressing the Travis failure is appreciated.

@sbrl
Copy link
Member

sbrl commented Oct 2, 2016

This looks like a good change.

After taking a look it looks like something is unhappy about the on_black bit in the config file.

@waldyrious
Copy link
Member Author

Yeah, I noticed that the problem is in the "on_black", but any guesses at how it can be fixed?

@sbrl
Copy link
Member

sbrl commented Oct 2, 2016

After some more digging I found a man page for the termcolor.py package that appears to be in use under the hood: https://pypi.python.org/pypi/termcolor

Here's what I think is an appropriate extract:

Text highlights:

  • on_grey
  • on_red
  • on_green
  • on_yellow
  • on_blue
  • on_magenta
  • on_cyan
  • on_white

Apparently on_black isn't supported. Theoretically speak there should be a transparent option in there somewhere, which should show in underlying terminal color (black on windows, but purple on Ubuntu)

@waldyrious
Copy link
Member Author

Thanks for the info! Ok, then let's just omit the on_black, which is a best-effort attempt at matching the node client's current color scheme (and an improvement at that over the current configuration).

@waldyrious
Copy link
Member Author

I need some help updating the test data, since it has the current colors hardcoded (ugh...)

@bwh1te
Copy link
Contributor

bwh1te commented Oct 3, 2016

Come on! Did you read the docs, guys? :) I had wrote about it in readme in February:

Background color: on_blue, on_cyan, on_magenta, on_white, on_grey, on_yellow, on_red, on_green

No on_black as you can see.

@waldyrious
Copy link
Member Author

I did read that @bwh1te, but assumed it was just an omission since that list didn't explicitly mention that it was exhaustive, and I didn't expect that termcolor implemented only a handful of colors rather than all combinations of foreground/background among the supported colors.

By the way, do you think you could lend a hand with fixing the test data?

@lots0logs lots0logs mentioned this pull request Jan 30, 2017
@waldyrious
Copy link
Member Author

Thanks @lots0logs for fixing the tests :)

@waldyrious waldyrious merged commit 00b45bc into master Jan 30, 2017
@waldyrious waldyrious deleted the default-colors branch January 30, 2017 09:45
@waldyrious
Copy link
Member Author

By the way, @lots0logs, does the failure with pypy3 seem like something that could be easily fixed? (Just to be clear, that failure is a sub-job of the tests for this PR, which passed since that environment is optional.)

@lots0logs
Copy link
Contributor

@waldyrious Hmm..looks like the version of python3 pypy currently used for the test is too old. Let's try using the latest...

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.

None yet

4 participants