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

Remove line cleaning with \r #108

Closed
wants to merge 16 commits into from
Closed

Remove line cleaning with \r #108

wants to merge 16 commits into from

Conversation

alonyb
Copy link

@alonyb alonyb commented Dec 11, 2020

This RP is to delete line cleaning on all spinner printing.

I think that way it is easier to implement in another code or line just calling it at the end.

I removed Windows validation, it works without it, it's the same.

And add a new break line at final of spinner process stop()

This PR is used in minikube code, you can see it here

@briandowns
Copy link
Owner

Thanks for submitting this. I'll review shortly. This might take a little time as I can't merge it until I get confirmation it works on Windows as well given the explicit windows code removal.

What problem is this solving?

@briandowns
Copy link
Owner

I'd prefer any output be performed explicitly by the user in the suffix, prefix, and finalMsg fields rather than built-in as this removes any choice from users who would prefer not to have those characters printed.

As for terminal checking, and the rest of this PR, I'll give it a look and test over the weekend. As I previously asked, can you pinpoint the issues that this is resolving? Would help in reviewing what is actually changing. Thanks!

@briandowns
Copy link
Owner

I ran a quick test on Linux (Ubuntu) and it seems to be broken in terms of expected output using the built-in terminal and Tilix as well as adding additional new line characters. I love getting community PR's but this changes quite a lot of base expectations and I'm still not sure what problems or issues it's fixing/resolving.

@briandowns
Copy link
Owner

Closing as questions haven't been answered, I don't know the problem it's solving, and it appears broken in certain regards. Please feel free to reopen but if you do, please answer the questions I've posed above.

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

Successfully merging this pull request may close these issues.

3 participants