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

Added an option to disable color highlighting #107

Merged
merged 4 commits into from
Oct 19, 2023

Conversation

Vedaant-Rajoo
Copy link
Contributor

@Vedaant-Rajoo Vedaant-Rajoo commented Oct 13, 2023

What does it do?

Disable color display when the output is not a tty.

Why the change?

Color gives weird ANSI polluted output for non tty screens. tldr is used not only in terminals but other places like code editors like vim and emacs where this behaviour might not be simplified.

How can this be tested?

tldr tldr > tldr.md
tldr --color tldr > tldr.md
tldr tldr

Where to start code review?

parser.c

Relevant tickets?

closes #35

Questions?

<Ask us anything!>

@kbdharun kbdharun self-assigned this Oct 14, 2023
Copy link
Member

@kbdharun kbdharun left a comment

Choose a reason for hiding this comment

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

LGTM, Thanks for your contribution and welcome to tldr.

Changes I made

When testing your code I got this error:

➜  tldr-c-client git:(feature/color-control) make
CC src/local.c
CC src/net.c
CC src/parser.c
CC src/tldr.c
src/tldr.c: In function ‘main’:
src/tldr.c:63:18: warning: implicit declaration of function ‘isatty’ [-Wimplicit-function-declaration]
   63 |     color_flag = isatty(fileno(stdout));
      |                  ^~~~~~
CC src/utils.c
LD tldr

I fixed it by including the #include <unistd.h> header.

Additionally, I made some indentation changes in code and added .vscode to the .gitignore file (I use VSCode as an IDE and it by default generates JSON configuration files inside the directory)

Now, make compiled fine and the outputted file is proper without any pollution.

➜  tldr-c-client git:(feature/color-control) make  
CC src/tldr.c
LD tldr
➜  tldr-c-client git:(feature/color-control) ✗ sudo make install
Place your right index finger on the fingerprint reader
install -d /usr/local/bin
install ./tldr /usr/local/bin
install -d /usr/local/share/man/man1
install man/tldr.1 /usr/local/share/man/man1

Output

tldr.txt

@Vedaant-Rajoo Feel free to review and comment on the changes I made.

Vedaant-Rajoo and others added 3 commits October 14, 2023 13:30
Disable color display when the output is not a tty.
This change should not break the current workflow
when tldr is used in terminals.
Resolves: tldr-pages#35

Author:    Vedaant-Rajoo <vedaant12345@gmail.com>
Co-authored-by: ksqsf <i@ksqsf.moe>
Date:      Fri Oct 13 18:15:39 2023 +0530
Signed-off-by: K.B.Dharun Krishna <kbdharunkrishna@gmail.com>
Signed-off-by: K.B.Dharun Krishna <kbdharunkrishna@gmail.com>
@SethFalco
Copy link
Member

Perhaps it's worth supporting NO_COLOR too while we're here? Or I could do it as a separate PR to streamline this one.

NO_COLOR is an informal standard/convention to disable color in command-line application output. It's usually implemented in color libraries, but since we don't use one we'd be responsible for doing it directly.

If NO_COLOR is set to any non-empty string, all colors are disabled.

@kbdharun
Copy link
Member

Perhaps it's worth supporting NO_COLOR too while we're here? Or I could do it as a separate PR to streamline this one.

NO_COLOR is an informal standard/convention to disable color in command-line application output. It's usually implemented in color libraries, but since we don't use one we'd be responsible for doing it directly.

If NO_COLOR is set to any non-empty string, all colors are disabled.

Agreed, feel free to modify the PR.

@SethFalco
Copy link
Member

I've added a commit, feedback welcome.

  • If NO_COLOR is set, don't display color unless overridden otherwise.
  • Meanwhile, the existing tty check is still there, but only performed if NO_COLOR is not set.

@SethFalco
Copy link
Member

SethFalco commented Oct 19, 2023

An issue I noticed with the branch though, it seems ./tldr -C tldr doesn't work though?

./tldr: unrecognized option '-C'

The long option works flawlessly, but before adding my commit, the short option wasn't working, I did not resolve this. 🤔

@kbdharun
Copy link
Member

n issue I noticed with the branch though, it seems ./tldr -C tldr doesn't work though?

./tldr: unrecognized option '-C'

The long option works flawlessly, but before adding my commit, the short option wasn't working, I did not resolve this. 🤔

Interesting, I will try replicating this.

@kbdharun
Copy link
Member

Update: Tested it right now, the force colour option works fine for me before your commit too.

I think this PR is GTG will merge it and focus on the other PR updating the man page.

@kbdharun kbdharun merged commit 804f408 into tldr-pages:main Oct 19, 2023
4 checks passed
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.

disable colors
4 participants