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

Adds '--clear-screen' option to clear screen between watch runs #3355

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

colinta
Copy link

@colinta colinta commented Nov 23, 2024

This is very slapped together, but I wanted it for myself and figured I'd start the conversation. If you are even open to mergin it let me know of any changes.

Usage:
npm test ava --watch --clear-scren 
What does this PR change?

Between test runs, it sends this.lineWriter.write(ansiEscapes.clearTerminal), which clears the screen and scrollback buffer on all the terminal apps I have handy.

I promoted this same behaviour in TypeScript, I guess it's my hill to die on 🤣 (in that case, --watch was already clearing the screen, this just changed/improved the behaviour).

Jest also clears the screen when using --watch. I prefer it, FWIW, but I've read the argument for keeping all those logs... I'm not convinced, but hey that's just me. I'd be happy w/ --clear-screen though it isn't exactly very discoverable atm.

WDYT?

@colinta colinta force-pushed the main branch 2 times, most recently from 3f24214 to 4674c0d Compare November 23, 2024 04:46
@colinta
Copy link
Author

colinta commented Nov 23, 2024

When I first wrote this, I used process.stderr.write('\x1b[2J\x1b[H\x1b[3J');, but that felt wrong given the use of this.lineWriter.write so I switched to that... but now it doesn't work, so I switched it back to process.stderr.write. 🤷‍♂️

Correction: it works fine using lineWriter.

@sindresorhus
Copy link
Member

I have no opinion whether this should be added, but the flag should be --clear-screen.

@colinta colinta changed the title Adds '--cls' option to clear screen between watch runs Adds '--clear-screen' option to clear screen between watch runs Nov 23, 2024
@colinta
Copy link
Author

colinta commented Nov 23, 2024

I'm not personally a fan of the more verbose --clear-screenI chose cls for what I thought was a good reason – but I won't quibble. 🙂 (edit: that article points out that in *nix, the equivalent command would be clear, but I like --clear even less than --clear-screen, so --clear-screen is a decent compromise at the end of the day, and in the absence of other arguments, verbose/clarity beats all)

I gave this a short alias of -C, btw. Like/dislike? I'm not stuck on it.

Copy link
Member

@novemberborn novemberborn left a comment

Choose a reason for hiding this comment

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

Yea I don't mind this. Our long-form flags are not abbreviated so 👍 on --clear-screen.

Do we want to keep this as a CLI flag only or also support it in the configuration?

Testing wise, this could be a permutation of

t.test('default reporter - watch mode run', run('watch'));
which should include the ansi code in the generated reporter logs. Let me know if you need a pointer on how to regenerate those, it's far from ideal.

@@ -128,6 +131,10 @@ export default class Reporter {
}

startRun(plan) {
if (this.clearScreen) {
this.lineWriter.write(ansiEscapes.clearTerminal);
}
Copy link
Member

Choose a reason for hiding this comment

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

Maybe move this down into the if (this.watching && !plan.firstRun) line below? It better connects clearScreen with watch mode plus maybe there's no need to clean the terminal on the first run? Or does that get weird?

Copy link
Author

Choose a reason for hiding this comment

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

I had it somewhere down there to start, but then it occurred to me that --clear-screen without --watch had no effect at all, which simply felt "unhandled". Why not have it clear the screen? 🤔

Since then I've been actually using it without the watch flag just as often as with it.

@colinta
Copy link
Author

colinta commented Nov 25, 2024

Thanks for the pointers, I'll try to get those tests updated, and yes having it config makes sense 👍

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.

3 participants