wp-cli / php-cli-tools

A collection of tools to help with PHP command line utilities
MIT License
673 stars 118 forks source link

\cli\progress\Bar enhancement: Allow updating _message on tick #125

Closed lf-jeremy closed 6 years ago

lf-jeremy commented 6 years ago

Inspired by the progress bar as implemented by the league/climate package, it'd be useful to allow updating the _message property displayed by a progress bar as it is working on different items being processed or hits different key steps of processing.

In their progress bar documentation, the updated message is passed as an optional second parameter of their equivalent tick method.

Using a similar method signature, this would not be a breaking change and would give the option for better visibility of work/steps/tasks being performed. I'm going to extend the \cli\progress\Bar class in the meantime, but it seems like it'd be useful in general use cases for anyone to use.

danielbachhuber commented 6 years ago

👍 We'd be amenable to a pull request if you care to submit one.

lf-jeremy commented 6 years ago

Just a side note:

While updating the examples script to include changing the progress bar message, I encountered an off-by-1 error in the existing example code.

The test_notify function has a default cycle of 1000000 iterations. The example constructors for \cli\Progress\bar objects pass a size of 1000000 steps. However, the for loop inside test_notify starts at 0 and goes to $cycle inclusive.

It's not noticeable at 1000000 iterations, but using a small number of iterations with a long duration, it becomes noticeable there is a mismatch. I have left the existing examples unchanged for now, but my added example is setup to avoid the problem until it has been discussed.

schlessera commented 6 years ago

@lf-jeremy Oh, good catch. I opened a separate bug report for this: https://github.com/wp-cli/php-cli-tools/issues/127

lf-jeremy commented 6 years ago

Fixed by #126