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

chore(increment): Handle the use case when increment value is not passed, but payload is #67

Closed

Conversation

ecdeveloper
Copy link

Right now, if I do something like:

b1.increment({ foo: 'bar' })

This breaks the progress bar.

It'd be nice if we could pass a payload to increment, without explicitly specifying the increment step.

@AndiDittrich
Copy link
Member

Hi @ecdeveloper ,

thanks for your contribution. in one of the last releases the update() method has been modified to allow a payload update without affecting the value. just pass null as first argument:

bar.update(null, {
x: "hello"
});

@ecdeveloper
Copy link
Author

ecdeveloper commented Apr 16, 2020

hey @AndiDittrich ,

In my use case I actually had to increment the progress, along with updating one of the extra values. So technically that would be bar.update(<currentValue> + 1, { x: 'hello' }), which imo is much cleaner to write as bar.increment({ x: 'hello'}).

@AndiDittrich
Copy link
Member

imho bar.increment(1, { x: 'hello'}) is also very clean

@ecdeveloper ecdeveloper deleted the chore/increment-improvement branch April 16, 2020 21:44
@ecdeveloper
Copy link
Author

@AndiDittrich I wouldn't argue it's clean, but in some cases it may just look ambiguous.

I just happened to notice a similar interface in another library - https://github.com/visionmedia/node-progress#custom-tokens. They let you do both. But I guess it's up to you :)

@AndiDittrich
Copy link
Member

i've added the same polymorphic style to update() and increment() - see bd09d07

@ecdeveloper
Copy link
Author

@AndiDittrich thanks. Just out of curiosity - why wouldn't you provide feedback so I could tweak this PR, and instead just rewrote this patch in a separate commit?

@AndiDittrich
Copy link
Member

it was much faster (only reason)

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.

None yet

2 participants