-
Notifications
You must be signed in to change notification settings - Fork 29
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 later_recurring #133
base: main
Are you sure you want to change the base?
add later_recurring #133
Conversation
- if the 'func()' returns a logical, then its return value is taken as a "continue" logical; that is, if 'func()' returns true, then continue, if false then stop (the recurrency is cancelled)
I believe the
@wch, is there something I need to do to the PR to fix that failure? |
@r2evans There is some documentation updates to be made. But the actions should be auto pushing it back. |
@schloerke if there's a problem that I can help speed-along with a manual push to my repo, let me know ... it seems that this set of GH actions is not behaving to your liking. |
@r2evans The GHA won't be able to push back changes to a forked repo (which is this PR's situation). GitHub says it is security issue when trying to push changes back to a forked repo from the main repo. I'll be working on a better error message in the morning. I'll tag you when it's ready. (Please don't fix the doc changes yet so I can continue testing in the morning) |
@r2evans The workflow has been updated. Please let me know if the new error message does not provide enough hints/instructions. Since the workflow has been updated, please fix any errors it reports. Thank you! |
Sorry @schloerke , I do not understand.
While I understand that something in the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to test-cancel.R
, can you add tests to make sure limit
and the return cancel function behave as expected? Thank you!
- change limit= default to Inf (NA no longer accepted) - add is_false (and one update to later.R) - update doc (details and example code)
** Growing pains, sorry. The If you merge the latest from main (or rebase), the error should go away. Feel free to ignore it as well. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the tests and extra usage of is_false()
!
Import the functionality of
pool::scheduleTaskRecurring
directly intolater
, with additional limits.This PR is a combination of things I like to do with polling background processes. I'm familiar (a little) with
coro
, I think this addition fits better here inlater
than there.Impetus: I have a bulk-query function that spawns a new process (
processx
callingsqlcmd
) that takes several minutes (data volume, not unexpected). As an optional courtesy in my bulk-query function, I poll the process state and, when exited, I display a message on the console (prompting the user to do something with the new data).So I do something akin to:
Having a never-ending recurring loop might be problematic (and concerning), so there are two additions to
pool::scheduleTaskRecurring
:func
: if the udf returnsFALSE
, then it is self-cancelling; if nothing (or not the literalFALSE
) is returned, it will not self-cancel;limit=NA
(the default) allows limitless execution, settinglimit=9
permits only 9 iterations before self-cancelling.