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

fix: task returned from content_render missing task_id #339

Merged
merged 6 commits into from
Nov 27, 2024
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions NEWS.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,11 @@
`user_role` parameters. For example, this allows you to find all licensed
users on a Connect server. (#311)

## Minor improvements and fixes

- The task returned by `content_render()` now has the expected `task_id`
parameter and is able to be polled. (#338)

# connectapi 0.4.0

## New features
Expand Down
3 changes: 3 additions & 0 deletions R/content.R
Original file line number Diff line number Diff line change
Expand Up @@ -1007,6 +1007,9 @@ content_render <- function(content, variant_key = NULL) {
target_variant <- get_variant(content, variant_key)
}
render_task <- target_variant$render()
# Tasks returned from variant render endpoint look different from those
# returned by deploy.
render_task$task_id <- render_task$id
Copy link
Collaborator

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-L69

Why doesn't that handle this for you already?

Copy link
Collaborator

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 from Task :/

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.

Copy link
Collaborator Author

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.


VariantTask$new(
connect = content$connect,
Expand Down
2 changes: 1 addition & 1 deletion R/deploy.R
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't technically need this change right? get_task() just yields self$task. (Not opposed to ripping out the unnecessary getters, but that seems better done in a separate refactor PR where you did them all.)

Copy link
Collaborator Author

@toph-allen toph-allen Nov 27, 2024

Choose a reason for hiding this comment

The 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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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"]]
Expand Down
1 change: 0 additions & 1 deletion R/parse.R
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,6 @@ tzone <- function(x) {
attr(x, "tzone")[[1]] %||% ""
}


new_datetime <- function(x = double(), tzone = "") {
tzone <- tzone %||% ""
if (is.integer(x)) {
Expand Down
9 changes: 9 additions & 0 deletions tests/integrated/test-content.R
Original file line number Diff line number Diff line change
Expand Up @@ -402,6 +402,15 @@ test_that("variant_render works", {
suppressMessages(poll_task(rnd2))
})

test_that("content_render works", {
rnd <- content_render(rmd_content)

expect_true(validate_R6_class(rnd, "VariantTask"))

# wait for tasks to complete...
suppressMessages(poll_task(rnd))
})

test_that("get_variant_renderings works", {
scoped_experimental_silence()

Expand Down
Loading