alleyinteractive / wp-bulk-task

A library to assist with running performant bulk tasks against WordPress database objects.
GNU General Public License v2.0
13 stars 0 forks source link

Rewind: Some indication that we are at the end of all processed item #5

Closed jmslbam closed 7 months ago

jmslbam commented 1 year ago

I was searching and searching and did some more searching... and then I used the --rewind flag and it started again :)

Currently it just says the same final message that the progress bar makes.

Bulk Task: Remove duplicate attachments 100% [==============] 0:00 / 0:00

Maybe a notices that says to make it more clear, like:

Already at the end, rewind to start over again.

How do you feel about that?

mboynes commented 1 year ago

hey @jmslbam, thanks for taking the time to suggest a feature! I hope you're enjoying using this library.

For posterity and to get on the same page, the library itself doesn't have support for a --rewind flag, that's merely a suggestion in the docs (and in the README's example), but it's up to the developer to implement. That's not to disregard your suggestion, but re-route it.

I agree, if the cursor is maxed out, the library could at least alert the user.

Perhaps something here that checks if $this->min_id >= $this->max_id and throws a helpful error, something like There is nothing to process. Do you need to reset your cursor?. Alternatively, we could prompt the user, The cursor is at {$this->min_id} and the highest ID in the database is {$this->max_id}, so nothing will be run. Would you like to reset the cursor before starting?

My one hesitation, at least with the prompt idea, is if the library would be starting to do too much. Leaving it to the developer to implement a cursor reset was a specific design decision to empower developers. If I let my imagination run wild, I could see a potential edge case where a process which automatically respawns a command could either get hung up on the prompt, or worse, run this in perpetuity if it automatically says YES.

@kevinfodness penny for your thoughts?

kevinfodness commented 1 year ago

Agree about providing feedback to the user - that would be helpful.

The intentional decision we made here was to not have a hard dependency on, or assume this library was being used with, WP_CLI. It could also be used with, say, a cron runner, or another type of task scheduler or queueing system. Therefore, the method in which the cursor would be reset could be different between those use cases (maybe resetting via WP_CLI, or maybe being reset in code via another mechanism, or within WP admin, etc).

That's also the reason why Progress is an interface - it can use the WP_CLI progress meter (and, indeed, is modeled after its structure) but could use a different type of progress bar, including one that would write its progress to the database to be viewable on a WP admin dashboard.

Because this library doesn't directly produce any output (progress bars are passed in via dependency injection, if the user chooses to use one) we would need to implement something similar for output. The library doesn't currently output anything other than progress, and that only if the implementer passes in a progress handler.

mboynes commented 1 year ago

Great points. Given all that, here's a pitch and a half: what if we calculated the max ID before getting the min id from the cursor, and we either filter the cursor's value or we fire an action that passes the max id and the cursor object, which would give the dev a chance to reset it before it's read.

jmslbam commented 1 year ago

Hi all! Thank you for taking the time to think about it.

While keeping the fact in mind that it could also be used in somewhere else. I was a bit to focussed on using it wihtin WP-CLI :). I will simply implement that feedback in my custom Command class which I extend. I already contains a bit of extra customized code which I use often.

I'm ok with closing this ticket and leaving it as little note for other developer 👍

renatonascalves commented 7 months ago

Resolving this issue.