-
Notifications
You must be signed in to change notification settings - Fork 130
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
Better color detection #111
Better color detection #111
Conversation
Oh and since this changes the CLI the completions for bash/zsh/fish should be updated as well. I can look into working on that, but I won't have time till at least a week from now and also haven't setup shell completions before if anyone more knowledgable can help. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, thanks for the PR! From the description your work looks very thorough 🙂
I'll try to review the PR this weekend, but can't promise anything.
I can look into working on that, but I won't have time till at least a week from now and also haven't setup shell completions before if anyone more knowledgable can help.
"Best effort" is OK (adding the strings without testing), since with #108 we'll auto-generate those completions anyways.
These changes were all tested exclusively on Linux since I don't have a rust dev environment setup for Windows and I don't have access to any other systems.
CI runs on macOS and Windows as well. Maybe we can create integration tests for some of these cases?
test_markdown_rendering seems to be failing because of changes to the tar page
That was fixed in #110, I simply forgot to merge that one. Sorry about that.
--color always (The previous behavior of tealdeer)
Will use styling as long as ANSI support is detected.
I wonder if always
should use ANSI support detection at all... That somehow seems to contradict the term "always".
Thinking back on it now it does seem deceptive to have a case where I should be able to find the time to:
over the next couple days. Just a bit busy with the end of the semester approaching after all |
0189ab1
to
c6cdddd
Compare
So I found time to go ahead and make the changes. I based the shell completions off the |
c6cdddd
to
19e2905
Compare
Saw this got some merge conflicts over time so I decided to go ahead and fix them. |
Great, thanks, and sorry for the delay! I applied rustfmt on the master branch, this should get rid of your first commit. There's also a conflict in |
I can fix it and rebase if you give me about a week (semester is busy atm). But if you don't mind doing it yourself and pushing the changes then feel free! |
This change makes the color for the "stale cache" warning follow `enable_styles`, like the rest of the styling.
19e2905
to
c840546
Compare
@LovecraftianHorror great, I went ahead and rebased your branch. I also squashed a few of the commits (e.g. the test fixes). Will try to review (and ideally merge) the PR tonight. |
c840546
to
dc6af5a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @LovecraftianHorror, this looks great 🙂 I only fixed a small nitpick (didn't want to bother you with a change request), now it should be ready to merge if CI passes.
There's one more open PR, if I get that reviewed soon I should be able to create a new release next week.
Hello, first time hacking at this codebase.
So the aim of this pull request is to address the issues mentioned in #81. This was done by adding in a command line flag
--color
with the optionsalways, auto, never
.--color <WHEN>
--color always
(The previous behavior oftealdeer
)Will use styling as long as ANSI support is detected.
--color auto
Will use color unless:
NO_COLOR
environment variable exists as is the spec listed on theNO_COLOR
website.stdout
(e.g. when the output is being piped to another program liketldr man | xclip -sel clip
). This is done with the help of the newly addedatty
lib since it promises cross platform detection for such cases.--color never
Disables colored output entirely.
Misc
check_cache
to make style checking consistent.test_markdown_rendering
seems to be failing because of changes to thetar
page compared totar-markdown.expected
that I left failing as it's outside the scope of this PR.