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

feat: make progressbar could update according to an interval or updat… #199

Merged
merged 5 commits into from
Sep 23, 2024

Conversation

chengxilo
Copy link
Contributor

Hi, I write some code to solve the problem that is happend to some issues, such as #166 and #141 .

According to the previous code. I set the progressbar to update per 100 * time.Millisecond by default. If user do not want to start a go routine and update it repeatly, it could be set to zero. It works pretty well and seems nice, you can try it with the code in #166 and #141 .

But this feature cannot pass the test because the \r escape character cannot be parsed if we use buffer. I do not know how to handle this kind of problem actually.

For example, this is an Failed test.
image

Maybe we need to create a function to handle this problem. In the test, use a buffer to cache the output ,and then use a function to parse the string to what we will see in the terminal.

This PR may need further development. Unless it will cause some of the test function noneffective.

@schollz
Copy link
Owner

schollz commented Sep 18, 2024

Thanks for the edit, it looks like it is failing tests?

@chengxilo
Copy link
Contributor Author

Yes. It is failing tests.

It's because I changed the default setting of the progress bar. If you used the spinner, than the progress bar will change by a time interval by default and it makes the output a great difference from the test.

In my opinion,the old way to change the spinner do really cannot change the spinner properly.

In the original code,the spinner change each 100ms, and if you don't render it, you cannot see that change. That's why those issue was caused.

For example,try this code:

package main

import (
    "fmt"
    "github.com/schollz/progressbar/v3"
    "time"
)

func main() {
    bar := progressbar.NewOptions(-1,
        progressbar.OptionSetDescription("test"),
        progressbar.OptionSpinnerType(7))
	for i := 0; i < 10; i++ {
		bar.Describe(fmt.Sprintf("test"))
		time.Sleep(400 * time.Millisecond)
	}

}

You will realized that spinner never changed. Becuase the spinner type 7: {"◐", "◓", "◑", "◒"}, it have 4 different character. So if the interval of render is 400ms, each time when we render it, the spinner would always be "◐".

I think it should be changed, the old logic is a defect for most of the users I guess.

I'm also wondering if I should change the default form, perhaps causing some negative effect the use of some developers in the past. But if you let me make a choice, I think I will change it. I don't think anyone would use the output of progressbar as a part of their logic in the application.🤔

And most of the failed tests are because of the extra output from the goroutine, which was cleaned by '\r \r'. Like my screenshot shows, I rendered more times and the output in the buffer would be much longer. But if it is output to stdout, they will be the same. I think the test should base on the extra output. For example, if we expect to see the progressbar to be cleaned. Than what we need is to check whether the actual output is empty, not check whether the string in the buffer is equals to this

"\r-  (10 B, 10 B/s, 10 it/s) [1s] " +
"\r                                \r"

I am currently struggle with it, I will create a project, create a tool which makes it possible for us to know what the output will be according to the buffer. Simulating the stdout, make those bytes cleaned by '\r \r' could be ignored.

Sorry about my barren language expression.🥲 I've already tried my best.

@schollz
Copy link
Owner

schollz commented Sep 19, 2024

Yeah I understand.

I would happily accept the change - as long as you update the tests to pass I would be able to merge.

@chengxilo
Copy link
Contributor Author

Thank you.🫡

@chengxilo
Copy link
Contributor Author

Well, I gonna say that a lot of words I've told about the tests are wrong. It's actually I've not realized that only when the max = -1 , the ignoreLength is true, than we should render the spinner.🤓 Confident and stupid.

@schollz
Copy link
Owner

schollz commented Sep 22, 2024

not stupid at all! this is a fantastic patch, great idea of breaking out the virtualterm package. I'd love to merge it. do you think its ready?

@chengxilo
Copy link
Contributor Author

Yes,hhh

@schollz
Copy link
Owner

schollz commented Sep 23, 2024

Amazing, thanks!!!

@schollz schollz merged commit f1b3580 into schollz:main Sep 23, 2024
1 check passed
@chengxilo chengxilo deleted the update-spinner branch September 23, 2024 13:12
renovate bot referenced this pull request in nobl9/sloctl Sep 24, 2024
This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
|
[github.com/schollz/progressbar/v3](https://github.com/schollz/progressbar)
| `v3.15.0` -> `v3.16.0` |
[![age](https://developer.mend.io/api/mc/badges/age/go/github.com%2fschollz%2fprogressbar%2fv3/v3.16.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://developer.mend.io/api/mc/badges/adoption/go/github.com%2fschollz%2fprogressbar%2fv3/v3.16.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://developer.mend.io/api/mc/badges/compatibility/go/github.com%2fschollz%2fprogressbar%2fv3/v3.15.0/v3.16.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://developer.mend.io/api/mc/badges/confidence/go/github.com%2fschollz%2fprogressbar%2fv3/v3.15.0/v3.16.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|

---

### Release Notes

<details>
<summary>schollz/progressbar
(github.com/schollz/progressbar/v3)</summary>

###
[`v3.16.0`](https://github.com/schollz/progressbar/releases/tag/v3.16.0)

[Compare
Source](https://github.com/schollz/progressbar/compare/v3.15.0...v3.16.0)

#### What's Changed

- feat: make progressbar could update according to an interval or updat…
by [@&#8203;chengxilo](https://github.com/chengxilo) in
[https://github.com/schollz/progressbar/pull/199](https://github.com/schollz/progressbar/pull/199)

**Full Changelog**:
schollz/progressbar@v3.15.0...v3.16.0

</details>

---

### Configuration

📅 **Schedule**: Branch creation - "after 10pm every weekday,before 5am
every weekday,every weekend" (UTC), Automerge - At any time (no schedule
defined).

🚦 **Automerge**: Enabled.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR was generated by [Mend Renovate](https://mend.io/renovate/).
View the [repository job
log](https://developer.mend.io/github/nobl9/sloctl).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzOC44MC4wIiwidXBkYXRlZEluVmVyIjoiMzguODAuMCIsInRhcmdldEJyYW5jaCI6Im1haW4iLCJsYWJlbHMiOlsiZGVwZW5kZW5jaWVzIiwiZ29sYW5nIiwicmVub3ZhdGUiXX0=-->

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
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.

2 participants