-
Notifications
You must be signed in to change notification settings - Fork 28.9k
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
watch: add CLI flag to preserve output #45717
Conversation
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.
Is clearing the terminal really a good default choice? Doesn't that potentially take away error messages etc., depending on the terminal host? What if stdout is not a TTY?
I guess you are right it could be inconvenient to have to add a flag in order to preserve the output, if suggested i could change the logic to preserving the output by default |
Have added the relevant tests and refactored the option flag, I believe its ready for a review Thank You! |
No strong opinion regarding what the default to be, but I do think we should support clearing |
I don't use watch mode and I am not familiar with the current implementation. cc'ing those who approved #44366: @aduh95 @benjamingr |
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.
I think it makes sense to have an option for this, I think changing the default can be done in a separate PR – this PR also adds --no-watch-preserve-output
flag so if we want to switch the default it's already a step in the right direction.
Two nits:
understood so if changing the default is needed we can just update the option name in a separate PR? |
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.
Would you be able to change the first commit message to use watch
subsystem instead of lib,src
? I suggest something like watch: add CLI flag to preserve output
. If not it's OK it can be done by whoever lands this PR, or I can do it if you prefer.
Yes, we would need to update the docs, and change the value from |
Understood, rebasing the commit |
Added a --watch-preserve-output flag to watch mode Fixes: nodejs#45713
Co-authored-by: Mohammed Keyvanzadeh <[email protected]>
Co-authored-by: Antoine du Hamel <[email protected]>
Co-authored-by: Antoine du Hamel <[email protected]>
Co-authored-by: Antoine du Hamel <[email protected]>
Co-authored-by: Antoine du Hamel <[email protected]>
Commit Queue failed- Loading data for nodejs/node/pull/45717 ✔ Done loading data for nodejs/node/pull/45717 ----------------------------------- PR info ------------------------------------ Title watch: add CLI flag to preserve output (#45717) Author Debadree Chatterjee (@debadree25) Branch debadree25:ft/add-preserve-output-flag -> nodejs:main Labels c++, author ready, needs-ci, commit-queue-squash Commits 12 - watch: add CLI flag to preserve output - test: add test for preserve-output - lib,src: fixed linting issues - lib,src: refactor naming of flag - doc: add option to doc - doc: fix option ordering - test: remove unnecessary console - lib: store clear output in a const - doc: update content - lib: refactor writing output - doc: update content - doc: update content Committers 1 - Debadree Chatterjee PR-URL: https://github.com/nodejs/node/pull/45717 Fixes: https://github.com/nodejs/node/issues/45713 Reviewed-By: Antoine du Hamel Reviewed-By: Moshe Atlow ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/45717 Fixes: https://github.com/nodejs/node/issues/45713 Reviewed-By: Antoine du Hamel Reviewed-By: Moshe Atlow -------------------------------------------------------------------------------- ℹ This PR was created on Fri, 02 Dec 2022 18:03:15 GMT ✔ Approvals: 2 ✔ - Antoine du Hamel (@aduh95) (TSC): https://github.com/nodejs/node/pull/45717#pullrequestreview-1212730014 ✔ - Moshe Atlow (@MoLow): https://github.com/nodejs/node/pull/45717#pullrequestreview-1212754092 ✔ Last GitHub CI successful ℹ Last Full PR CI on 2022-12-11T17:46:19Z: https://ci.nodejs.org/job/node-test-pull-request/48410/ - Querying data for job/node-test-pull-request/48410/ ✔ Last Jenkins CI successful -------------------------------------------------------------------------------- ✔ No git cherry-pick in progress ✔ No git am in progress ✔ No git rebase in progress -------------------------------------------------------------------------------- - Bringing origin/main up to date... From https://github.com/nodejs/node * branch main -> FETCH_HEAD ✔ origin/main is now up-to-date - Downloading patch for 45717 From https://github.com/nodejs/node * branch refs/pull/45717/merge -> FETCH_HEAD ✔ Fetched commits as 22dc987fde29..dafdb5827c39 -------------------------------------------------------------------------------- [main 23433fbbd4] watch: add CLI flag to preserve output Author: Debadree Chatterjee Date: Fri Dec 2 23:26:43 2022 +0530 3 files changed, 9 insertions(+), 3 deletions(-) [main cb41045603] test: add test for preserve-output Author: Debadree Chatterjee Date: Sat Dec 3 00:27:34 2022 +0530 1 file changed, 26 insertions(+) [main 7138359251] lib,src: fixed linting issues Author: Debadree Chatterjee Date: Sat Dec 3 00:27:59 2022 +0530 2 files changed, 5 insertions(+), 3 deletions(-) [main 41fc6d8488] lib,src: refactor naming of flag Author: Debadree Chatterjee Date: Sat Dec 3 01:02:40 2022 +0530 4 files changed, 7 insertions(+), 7 deletions(-) [main 6b6b34a473] doc: add option to doc Author: Debadree Chatterjee Date: Sat Dec 3 01:02:59 2022 +0530 1 file changed, 9 insertions(+) [main d6020e08a1] doc: fix option ordering Author: Debadree Chatterjee Date: Sat Dec 3 01:10:07 2022 +0530 1 file changed, 1 insertion(+), 1 deletion(-) [main 7f49b18f72] test: remove unnecessary console Author: Debadree Chatterjee Date: Sat Dec 3 01:45:10 2022 +0530 1 file changed, 1 deletion(-) [main b16f74385c] lib: store clear output in a const Author: Debadree Chatterjee Date: Sat Dec 3 23:14:55 2022 +0530 1 file changed, 2 insertions(+), 1 deletion(-) [main d52b3f463f] doc: update content Author: Debadree Chatterjee Date: Sun Dec 11 22:44:53 2022 +0530 1 file changed, 1 insertion(+), 1 deletion(-) [main 56a8f68c6e] lib: refactor writing output Author: Debadree Chatterjee Date: Sun Dec 11 22:45:42 2022 +0530 1 file changed, 2 insertions(+), 4 deletions(-) [main 0185245962] doc: update content Author: Debadree Chatterjee Date: Sun Dec 11 23:00:25 2022 +0530 1 file changed, 1 insertion(+), 1 deletion(-) [main 00b1a164a5] doc: update content Author: Debadree Chatterjee Date: Sun Dec 11 23:00:44 2022 +0530 1 file changed, 1 insertion(+), 1 deletion(-) ✔ Patches applied There are 12 commits in the PR. Attempting to fixup everything into first commit. [main 885f461251] watch: add CLI flag to preserve output Author: Debadree Chatterjee Date: Fri Dec 2 23:26:43 2022 +0530 5 files changed, 43 insertions(+), 2 deletions(-) ✖ Git found no trailers in the original commit message, but 'Fixes: https://github.com/nodejs/node/issues/45713' is present and should be a trailer.https://github.com/nodejs/node/actions/runs/3670813383 |
Landed in 79a977a, thanks for the contribution 🎉 |
Thank you so much! thank you for your time too! |
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.
I don't like adding these many flags to Node's core executable. I would prefer it if this was an option to the existing flags.
The actual changes lgtm
@benjamingr Agreed. Maybe an interface similar to python's |
Fixes: nodejs#45713 PR-URL: nodejs#45717 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Moshe Atlow <[email protected]>
Fixes: #45713 PR-URL: #45717 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Moshe Atlow <[email protected]>
Fixes: #45713 PR-URL: #45717 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Moshe Atlow <[email protected]>
Fixes: #45713 PR-URL: #45717 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Moshe Atlow <[email protected]>
Fixes: #45713 PR-URL: #45717 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Moshe Atlow <[email protected]>
Fixes: #45713 PR-URL: #45717 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Moshe Atlow <[email protected]>
Fixes: #45713 PR-URL: #45717 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Moshe Atlow <[email protected]>
Fixes: #45713 PR-URL: #45717 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Moshe Atlow <[email protected]>
PR-URL: #54328 Refs: #45717 Reviewed-By: Jake Yuesong Li <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Debadree Chatterjee <[email protected]> Reviewed-By: Ulises Gascón <[email protected]> Reviewed-By: Moshe Atlow <[email protected]>
PR-URL: #54328 Refs: #45717 Reviewed-By: Jake Yuesong Li <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Debadree Chatterjee <[email protected]> Reviewed-By: Ulises Gascón <[email protected]> Reviewed-By: Moshe Atlow <[email protected]>
PR-URL: #54328 Refs: #45717 Reviewed-By: Jake Yuesong Li <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Debadree Chatterjee <[email protected]> Reviewed-By: Ulises Gascón <[email protected]> Reviewed-By: Moshe Atlow <[email protected]>
Added a --preserve-output flag to watch mode
Fixes: #45713