-
Notifications
You must be signed in to change notification settings - Fork 26
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
fix: task returned from content_render
missing task_id
#339
Changes from 5 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -580,7 +580,7 @@ poll_task <- function(task, wait = 1, callback = message) { | |
code <- -1 | ||
first <- 0 | ||
while (!finished) { | ||
task_data <- con$task(task$get_task()$task_id, wait = wait, first = first) | ||
task_data <- con$task(task$task$task_id, wait = wait, first = first) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You don't technically need this change right? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Correct, I don't technically need it; happy to revert if you think it's bad to change it here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree, for things like this which are easily greppable, no need to do it piecemeal. |
||
finished <- task_data[["finished"]] | ||
code <- task_data[["code"]] | ||
first <- task_data[["last"]] | ||
|
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.
I see this same logic in the Task
initialize
method: https://github.com/rstudio/connectapi/blob/main/R/deploy.R#L67-L69Why doesn't that handle this for you already?
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.
Right, because of course
VariantTask
doesn't inherit fromTask
:/I think the better fix would be to push this adapting into the
VariantTask
init method. Even better would be to look to fix the inheritance of all of these apparent (but not actual) Task subclasses, but that could be a followup--I'm not trying to derail a bugfix by tempting you into refactoring all the things.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.
Yeah lol. But good point, I'll put it into
VariantTask
and rip out the other place that also does it.I don't think
ContentTask
needs it because I don't think there are any content APIs that return this form.