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

if stdout is not a tty, replace clack.spinner by console.log #1473

Merged
merged 1 commit into from
Jun 28, 2024
Merged

Conversation

Fil
Copy link
Contributor

@Fil Fil commented Jun 20, 2024

closes #1447
closes #1446

I'm not happy that I had to make c8 specifically ignore the non-coverage of the non-tty output tests, but I did not find a better way. (solved in the new approach, see below)

@Fil Fil requested a review from mootari June 20, 2024 13:24
@mythmon
Copy link
Contributor

mythmon commented Jun 26, 2024

Generally we've been pushing these tty checks higher in the call stack, so that we can have different messages on non-TTYs. In the deploy effects we have a isTty variable that we can use in tests to test both kinds of messages. I'd prefer if we keep that pattern instead of making Clack sometimes behave differently.

@Fil
Copy link
Contributor Author

Fil commented Jun 26, 2024

isTty tests whether stdin is a tty, which will orient the questions one way or the other.

This by contrast is checking if stdout is a tty, so that we know not to output control characters; this is similar to what we do with color() in

return process.stdout.isTTY ? (text: string) => `\x1b[${code}m${text}\x1b[${reset}m` : String;

(In a sense, isTty is poorly named.)

@mythmon
Copy link
Contributor

mythmon commented Jun 26, 2024

I suppose we should add isStdinTty and isStdoutTty (I'd keep a isTty that is equal to isStdinTty && isStdoutTty too).

My point was more that we should push this up the stack, and leverage the effect system instead of doing it at import time.

@Fil
Copy link
Contributor Author

Fil commented Jun 28, 2024

@mythmon I've tried a different approach, hopefully closer to what you had in mind?

Copy link
Contributor

@mythmon mythmon left a comment

Choose a reason for hiding this comment

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

I think that's better. Good enough to merge at least so our CI logs look better.

@Fil Fil merged commit 831ec8a into main Jun 28, 2024
4 checks passed
@Fil Fil deleted the fil/tty branch June 28, 2024 17:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants