Fix stdout color support check

[osa1]

Currently canUseSpecialChars returns true when not in Windows and not in tests of the package itself.

This causes color codes to be printed when stdout is redirected or piped.

Fix it by checking if stdout is a tty. If it is, then we assume that most terminals today support colors and return true.

(We can't use the standard library supportsAnsiEscapes as it only returns true when $TERM is xterm, see
dart-lang/sdk#31606.)

Also fix the extended reporter's color support flag initialization, which was hard-coded as true.

Fixes #19.

[github-actions]

PR Health

Changelog Entry ✔️

Package	Changed Files

Changes to files need to be accounted for in their respective changelogs.

This check can be disabled by tagging the PR with skip-changelog-check.

[jakemac53]

This looks like it will cause a regression on windows by disabling it always.

[jakemac53]

Note that the windows implementation of this is not checking the $TERM variable, that only happens on mac and linux.

[osa1]

I'll update this check as before on Windows.

Mac has the same issue as Linux as it compares the TERM with a 4 known TERM values that no one uses. (in TermIsKnownToSupportAnsi)

[jakemac53]

The fix should just be to delete this line entirely afaik

[natebosch]

Yeah based on the discussion in the SDK issue and the linked stack overflow thread I'm inclined to change this to just the stdioType(stdout) == StdioType.terminal check and our testing workaround. I think we can drop the windows specific behavior. I don't think we need to wait for a decision SDK changes, we can make the update here now.

I'm also comfortable with the risk of making this change now. I think it's unlikely to break any workflows, and if it does I think it's OK to require a --no-color or --color flag to override.

[jakemac53]

In general, if we believe this is a better way of checking for ansii escape code support, then this should be the std library implementation? I am pretty hesitant to have special behavior here.

What happens if you just remove the !Platform.isWindows part instead?

[osa1]

What happens if you just remove the !Platform.isWindows part instead?

The problem is not with the Windows check. There are two problems:

1. Initialization of the color flag is incorrect in ExpandedReporter code. It's hard-coded as true.

2. supportsAnsiEscapes will never return true on most modern terminals: https://github.com/dart-lang/sdk/blob/2c0648dd7d54413284ab35932bacc572d297cc02/runtime/bin/stdio_linux.cc#L105-L114

   For example, I'm using alacritty, which is not covered by this implementation.

I can't fix just (1) because then it starts using the wrong code in (2) and I never get any colors. So this fixes both in one PR.

I can't separate the two if that's helpful, but we would need to merge (2) first, then (1), to avoid having a state in between that's even worse than the current state.

In general, if we believe this is a better way of checking for ansii escape code support, then this should be the std library implementation? I am pretty hesitant to have special behavior here.

See github.com/dart-lang/sdk/issues/31606. We could implement this properly in the standard library but it won't be trivial.

Instead the applications can assume that all ttys support colors, like we do here.

FWIW I did the same in the SDK's test runner a while ago. We use the same "is tty" check there to enable colors.

As I say in the issue, checking color support is not trivial (need to find and parse terminfo), and I doubt there are terminals in use today that don't support colors. Even if the standard library doesn't want to assume all ttys support colors I think we can safely assume it in the applications.

[jakemac53]

The problem is not with the Windows check. There are two problems:

That is half the problem, the current implementation doesn't even attempt to check for support on non-windows. I am in favor of removing that, fwiw. Your current change makes things worse on windows though by always disabling ansii escape codes on windows, if I read it correctly.

• Initialization of the color flag is incorrect in ExpandedReporter code. It's hard-coded as true.

Sure this seems like it should be fixed.

supportsAnsiEscapes will never return true on most modern terminals: https://github.com/dart-lang/sdk/blob/2c0648dd7d54413284ab35932bacc572d297cc02/runtime/bin/stdio_linux.cc#L105-L114

If the better implementation here is just to check if we are attached to any terminal, we should change the SDK implementation to that. We don't have to do a perfect implementation before making it better than what it is today.

Otherwise, the argument here is basically that nobody should use that function at all, and so why does it exist? Should it be deprecated?

[osa1]

Otherwise, the argument here is basically that nobody should use that function at all, and so why does it exist? Should it be deprecated?

Yes, I said the same thing in the link. It's been broken since at least 2017.

[natebosch]

We can't import this here, this library must be cross platform and configuration.dart imports dart:io.

We will need a configurable import somewhere if we want to avoid hardcoding this setting - and we'll need to still hardcode it for platforms without dart:io. I think adding a new library just to handle this indirection is OK.

[natebosch]

While we're here we should also add support for checking a NO_COLOR environment variable.

dart-lang/sdk#31606 (comment)

https://no-color.org/

BTW I think it'll be easier to land if the 2 changes are split up. We'll need to make sure we're compatible with google3 when we introduce new configurable imports.