Start spinner only when the stdout file descriptor is a terminal #131
Conversation
|
Thanks for the addition! |
|
@briandowns We at GitHub CLI love the spinner library, but I must voice my disappointment with the fact that a massive functional change like this one made it into a patch release (v1.18.1). |
|
@mislav I had made this patch in my project to prevent the spinner from starting when I wrote to a file. What is the case you want the spinner to start when you are not writing to the stdout? If it is the stderr, I can add that as well... |
|
@dwertent I think this impacts more than just writing to files. If you ran a task in Jenkins, for example, I believe this change would also prevent the spinner from outputting any information there too, as that's technically not a TTY. There are ways you could use the spinner in that non-TTY setup where the output wouldn't look bad, but now that's been effectively broken that in a patch release. Not that we need feature parity between the different spinners in our ecosystem, but it might be better to extend this change to behave more like my spinner (yacspin). Specifically this semantic: https://github.com/theckman/yacspin#supporting-non-interactive-tty-output-targets This allows you to continue supporting folks not running the code within a TTY, while providing a pretty good end user experience. |
|
@dwertent I get what this change does, and I don't blame you for contributing it. I just don't think a functional change should have been a patch release. Since this change is backwards-incompatible (now there is added detection on |
|
@mislav Happy to make adjustments. |
|
This also broke tests (PS, you have |
|
Thanks for pointing that out @QuLogic . I've updated the tests for now and moved to CicleCI. I'll clean things up a bit more tomorrow. |
This isn't a breaking change, but relates to briandowns#131.
No description provided.