Conversation
rhysd
left a comment
There was a problem hiding this comment.
Basically good 👍 except for animation. Would you take a look at my comment?
| sp=${{sp#?}}${{sp%???}} | ||
| sleep 0.1 | ||
| done | ||
| }} |
There was a problem hiding this comment.
I don't think animation is helpful for users since users since users already know what script is being run. And animation may cause some junks on some terminals. I want to choose keeping generated script simple here.
There was a problem hiding this comment.
I would argue the animation is essential since it shows the process isn't hung - some of those commands can take a looong time to finish. The actual animation doesn't really matter I just added the simplest I could find. As for the terminals, it should all be posix compliant which should make it fine with linux terminals and I've tested on windows with Git Bash, Powershell and good old cmd and they all show it just fine. Still, keyword is 'should'.
There was a problem hiding this comment.
I would argue the animation is essential since it shows the process isn't hung
Good point which I didn't care. I'm not understanding why user can know the process is not hung here. For example, let's say cargo test run by hook script hangs. Then the spinner will keep showing the animation. Rather than capturing output for spin animation, showing the log directly looks better way for me. cargo test shows output for each test case execution. So user can know test is still going ahead or is stuck at some test case. I think original husky does not hide output from underlying commands execution for check.
There was a problem hiding this comment.
Ohh right, for me showing everything just looked too cluttered :P Is there anything you want to keep from this pr?
coloredas a build dependency)