-
Notifications
You must be signed in to change notification settings - Fork 508
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
Add option to not show progress animations in logs #942
base: master
Are you sure you want to change the base?
Add option to not show progress animations in logs #942
Conversation
Add --noProgress flag that replaces the progress animations in the log with a simple log of the message. The progress animations will also be removed when the environment is not a TTY and when running in CI.
This comment has been minimized.
This comment has been minimized.
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.
LGTM
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.
To clarify, this is not a full, formal review, and normally I would not review until it is full, but there's a handful of issues (and improvements) with the PR as is:
- Docs were not updated
- The comment in
SharedOptions
is not correct (changingnoProgress
in the config does not change it, so the word "Should" is confusing and potentially misleading; "Is output progress being shown?" would be better) - Only 1 command was updated (
build
andwatch
should both be updated) - The
isTTY
portion is a breaking change and therefore can only go out in a breaking release. I would recommend that part be separated into a different PR as such. - I think this check should be embedded into
createProgressEstimator
instead of in individual uses. That will also make it useful when other functions usecreateProgressEstimator
- Typings could be improved/more specific (
Promise<any>
instead ofunknown
) - Is there a way to infer Lerna's current progress output status? That way there would not necessarily be a need for a new flag, which is what I was hoping to avoid in the issue
Thanks @agilgur5, I'll work on addressing those issues. |
A few comments on my last commit:
Since the option is a negative, I went with "Are output progress animations disabled?". Is that ok?
I've added the option to
It's now removed from this PR. I'll open a draft PR with that change separately.
In my use case I'm not using Lerna, so I'm hoping we can add this new flag. |
Fixes #689.
Adds a --noProgress flag (based on lerna's
--no-progress
, but made camelCase to match existing flags). When used, theprogress-estimator
logger is replaced by a simpleconsole.log
of the message. The progress animations will also be removed when the environment is not a TTY and when running in CI.Thanks for a great tool, and let me know if there's anything I can do to improve this PR.