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

Disable spinner when running in CI #356

Merged
merged 4 commits into from
Feb 13, 2023

Conversation

todor-a
Copy link

@todor-a todor-a commented Feb 3, 2023

Added support for disabling the spinner via an env var to make the output more CI-friendly.

For that, I updated commander to version 8.2.0, which added support for option values via env vars.

@todor-a todor-a force-pushed the expose-no-spinner-env-var branch from 11c6e19 to 9b41fe4 Compare February 3, 2023 07:39
@todor-a todor-a force-pushed the expose-no-spinner-env-var branch from 9b41fe4 to 3a7d544 Compare February 3, 2023 07:46
@todor-a todor-a changed the title disable spinner Provide a way to disable spinner via an env variable Feb 3, 2023
@kamiazya
Copy link
Collaborator

kamiazya commented Feb 3, 2023

@tdranv
Thanks for the PR!

If we are talking about CI considerations, how about using the ci-info package?

It seems preferable for users to be able to use it without knowing that the option exists.

@todor-a
Copy link
Author

todor-a commented Feb 3, 2023

Hey, thanks for the prompt response.

Do I understand correctly - you are suggesting disabling the spinner in general, if the command is running in a CI?

@kamiazya
Copy link
Collaborator

kamiazya commented Feb 3, 2023

Yes, I could not think of a case where a spinner would be effective in CI, so I thought it would be acceptable to disable the spinner in CI.

Any cases to consider?

@todor-a
Copy link
Author

todor-a commented Feb 3, 2023

Sounds reasonable.

We should definitely add it to the documentation tho.

@todor-a
Copy link
Author

todor-a commented Feb 3, 2023

I've made the requested changes. Let me know what you think when you get a chance, @kamiazya.

Not sure where to add the disclaimer in the readme tho. 🤔

@todor-a todor-a changed the title Provide a way to disable spinner via an env variable Disable spinner when running in CI Feb 3, 2023
@kamiazya kamiazya self-requested a review February 3, 2023 09:54
bin/cli.js Outdated Show resolved Hide resolved
@todor-a todor-a requested a review from kamiazya February 6, 2023 10:16
Copy link
Collaborator

@kamiazya kamiazya left a comment

Choose a reason for hiding this comment

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

LTGM!

I have verified that it works and that there are no problems.

@kamiazya kamiazya merged commit 7f64edb into pahen:master Feb 13, 2023
@kamiazya kamiazya self-assigned this Feb 13, 2023
@kamiazya kamiazya added this to the [Planning]6.1.0 milestone Feb 13, 2023
@todor-a todor-a deleted the expose-no-spinner-env-var branch February 15, 2023 06:49
@todor-a
Copy link
Author

todor-a commented Mar 10, 2023

Hey, @kamiazya. Do you plan to release a new version soon? Thank you! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants