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

Add emptyOnZero option to display total:0 bars as empty, not full #42

Merged
merged 2 commits into from
Sep 28, 2019
Merged

Add emptyOnZero option to display total:0 bars as empty, not full #42

merged 2 commits into from
Sep 28, 2019

Conversation

nickcmaynard
Copy link
Contributor

... also add devDependencies so npm run test works.

@AndiDittrich
Copy link
Member

Hi @nickcmaynard

i'll check this soon - there were a few reasons in the past to set the value to 1.0

@nickcmaynard
Copy link
Contributor Author

Great stuff - thanks. This defaults to current behaviour, so I hope this should be fairly low impact.

@AndiDittrich
Copy link
Member

in which cases does the unexpected behaviour appear ?
i can only reproduce the 1.0 fallback in case total is set to 0:

Example 1 - total=100

const b1 = new _progress.Bar();
b1.start(100, 0);
 $ node dev.js 
progress [----------------------------------------] 0% | ETA: 0s | 0/200

Example 1 - total=0

const b1 = new _progress.Bar();
b1.start(0, 0);
 $ node dev.js 
progress [========================================] 100% | ETA: 0s | 0/0

@AndiDittrich AndiDittrich merged commit c60f1e5 into npkgz:master Sep 28, 2019
@AndiDittrich AndiDittrich added enhancement validated issue validated by maintainer labels Sep 28, 2019
@nickcmaynard
Copy link
Contributor Author

in which cases does the unexpected behaviour appear ?
i can only reproduce the 1.0 fallback in case total is set to 0:

Yes - precisely this case. My use case pre-initialises a number of progress bars - "stages" - but the size of those stages isn't known until later in the execution.

Now, I could pre-inititialise total to a non-zero value, but that would erroneously imply it's correct.

Thank you for merging :)

@AndiDittrich
Copy link
Member

ok, i've never projected such use case

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

Successfully merging this pull request may close these issues.

None yet

2 participants