search

briandowns / spinner

Go (golang) package with 90 configurable terminal spinner/progress indicators.
Apache License 2.0
2.33k stars 129 forks source link

Start spinner only when the stdout file descriptor is a terminal #131

Closed dwertent closed 2 years ago

briandowns commented 2 years ago

Thanks for the addition!

mislav commented 2 years ago

@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).

dwertent commented 2 years ago

@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...

theckman commented 2 years ago

@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.

mislav commented 2 years ago

@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 os.Stdout where before there was none), I would argue this should have been v1.19 or even v2.

briandowns commented 2 years ago

@mislav Happy to make adjustments.

QuLogic commented 2 years ago

This also broke tests (PS, you have .travis.yml, but it doesn't run because Travis is dead for all intents and purposes.)

briandowns commented 2 years ago

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.