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

Accept more forced color values #156

Closed
wants to merge 3 commits into from

Conversation

sschuberth
Copy link
Contributor

While at it, extract color values for TERM / COLORTERM to variables.

@sschuberth
Copy link
Contributor Author

@ajalt, ./gradlew :extensions:mordant-coroutines:allTests also fails for me in master, so I guess the failure is not related to my changes?

@ajalt
Copy link
Owner

ajalt commented Feb 21, 2024

I'm -1 on this. FORCE_COLOR and COLORTERM are standard-ish variablers used by other libraries. I don't want really want mordant to behave differently than everyone else.

@sschuberth
Copy link
Contributor Author

FORCE_COLOR and COLORTERM are standard-ish variablers used by other libraries. I don't want really want mordant to behave differently than everyone else.

I don't see a problem with being lenient WRT the accepted values. I mean, we're not setting these variables and expecting other applications to adhere to that.

Also, "24bits" (plural) is already accepted for COLORTERM although it's not specified e.g. at http://jdebp.uk/Softwares/nosh/guide/TerminalCapabilities.html.

Finally, this

// A lot of npm packages support the FORCE_COLOR envvar, although they all look for
// different values. We try to support them all.
else -> when (getEnv("FORCE_COLOR")?.lowercase()) {

says "We try to support them all", so I really believe this should be fine.

@sschuberth
Copy link
Contributor Author

sschuberth commented Feb 21, 2024

To give you more background, this is to make this work-around function better: Apparently, TERM does not support truecolor values, so we need to preferably look at COLORTERM to set FORCE_COLOR, but the latter should support all COLORTERM values then.

@sschuberth sschuberth force-pushed the forced-truecolor branch 2 times, most recently from 20cbdd4 to a32a5ed Compare February 25, 2024 22:10
@sschuberth
Copy link
Contributor Author

@ajalt, mind having another look? I've split the commit into two to make the intent more clear.

@ajalt
Copy link
Owner

ajalt commented Mar 9, 2024

So i looked at the workaround you posted and I'm a little confused: why are you copying COLORTERM into FORCE_COLOR rather than passing COLORTERM through?

Extend the color variables to leniently accept more color values. So
besides only `FORCE_COLOR=256color`, also accept e.g.
`FORCE_COLOR=256colors` to force 256 colors. Similarly, besides
`FORCE_COLOR=truecolor`, accept e.g. `FORCE_COLOR=24bits` to force
true colors.
@sschuberth
Copy link
Contributor Author

why are you copying COLORTERM into FORCE_COLOR rather than passing COLORTERM through?

Not sure what you mean by "copying". Anyway, I've split into yet one more commit and hopefully clarified in the respective commits messages.

@ajalt
Copy link
Owner

ajalt commented Mar 13, 2024

Thanks for the additional clarification, but I'm still not in favor of this change. In the next major release, I plan to move in the opposite direction: I want to accept only the standard values of COLORTERM and FORCE_COLOR, rather than making up a bunch of new values and muddying the waters further.

@ajalt ajalt closed this Mar 13, 2024
@sschuberth
Copy link
Contributor Author

Thanks for the additional clarification, but I'm still not in favor of this change.

I've split my commits so that it could still make sense to just pick the first commit of this PR. Feel free to do that.

I want to accept only the standard values of COLORTERM and FORCE_COLOR

Would you have a pointer to what the definite / standardized values of these are? To me, it do es not seem to clearly specified anywhere, which was my motivation to be lenient in the accepted values.

@sschuberth sschuberth deleted the forced-truecolor branch March 13, 2024 19:58
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

2 participants