search

tldr-pages / tldr-c-client

C command-line client for tldr pages 📚
MIT License
299 stars 50 forks source link

add option --color and do not show colors when the output is not a tty #106

Closed ksqsf closed 11 months ago

ksqsf commented 1 year ago

What does it do?

Disable color display when the output is not a tty.

This change should not break the current workflow when tldr is used in terminals, but perhaps can break some CI runs (though unlikely), or perhaps someone's Emacs config that assumes color output. In that case, the users can pass --color to force color output to get the old behavior.

Why the change?

For example, when I call in emacs M-! RET tldr <cmd> RET, the display is very unreadable.

image

After the change:

image

See also #35.

How can this be tested?

  1. tldr cmd > file : no color
  2. tldr cmd : has color
  3. tldr --color cmd > file : has color
  4. tldr cmd | cat : no color
  5. tldr --color cmd | cat : has color

Where to start code review?

getopt options, and color_flag

Relevant tickets?

35

Questions?

It is unclear whether to pass color_flag to print_localpage. For now, I simply passed 1 to preserve compatibility.

kbdharun commented 11 months ago

Thanks for your contribution and welcome to tldr, this PR is being superseded by #107. Feel free to check it out.

ksqsf commented 11 months ago

I see you merged some PRs on Aug 20, so presumably you also reviewed this PR. I'm genuinely curious which part of my patch is not so good, so bad that you decided to defer it until someone else picks #35 up?

kbdharun commented 11 months ago

I see you merged some PRs on Aug 20, so presumably you also reviewed this PR. I'm genuinely curious which part of my patch is not so good, so bad that you decided to defer it until someone else picks #35 up?

Hi, sorry for closing this PR, it isn't that your patch is bad (it is indeed excellent work). I didn't notice it when I was first notified (as I get a lot of notifications from other repos I have on watch), just noticed it when I made changes to the other PR, that's why I closed this one. I will add you as a coauthor over there.

Edit. The TLDR C client is currently undermaintained, we are looking for new maintainers to make it comply with the recent specifications. Some of the members from the Org like myself occasionally visit the repo upon notification from PRs/issues.

kbdharun commented 11 months ago

If you don't mind, can you share your commit mail? So that I can add you as a coauthor over there.

kbdharun commented 11 months ago

Done