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

Image serializer #518

Merged
merged 19 commits into from Jun 11, 2020
Merged

Image serializer #518

merged 19 commits into from Jun 11, 2020

Conversation

meztez
Copy link
Collaborator

@meztez meztez commented Mar 29, 2020

Suggestion to fix issue #517.

Modified images handling to serialize during serialization step instead of postexec step

Comments welcome.

PR task list:

  • Update NEWS
  • Add tests
  • Update documentation with devtools::document()

@CLAassistant
Copy link

CLAassistant commented Mar 29, 2020

CLA assistant check
All committers have signed the CLA.

@meztez meztez changed the title Image serializer Image serializer issue [#517] Apr 18, 2020
@meztez meztez changed the title Image serializer issue [#517] Image serializer Apr 19, 2020
@schloerke schloerke added this to the v0.5.0 - Next CRAN release milestone Jun 8, 2020
@schloerke
Copy link
Collaborator

schloerke commented Jun 8, 2020

Seems odd to set the res$serializer in a post key val.

Note: Look into pre/post and what they mean

@schloerke schloerke added the priority: high Must be fixed before next release label Jun 8, 2020
@meztez
Copy link
Collaborator Author

meztez commented Jun 8, 2020

I agree, image handling seems to be its own thing. Maybe it could be refactored to match the rest of the components?

@meztez
Copy link
Collaborator Author

meztez commented Jun 10, 2020

I've played with other alternatives (separate image serializer, adding a hook stage to registerHook, adding rookEnv to serializer call). This is still the simpliest way to accomplish it without modifying too much of the existing code.

Otherwise you have to hit the serve function to prevent it from passing it through the default json serializer and other shenanigans.

I'm wondering if we go simple or I should edit more code.

I'll provide the alternative using serializer_content_type

@schloerke
Copy link
Collaborator

schloerke commented Jun 10, 2020

Ah! I think I understood why the original setup was confusing to me.

Let's

  • rename pre to preserializer (to be explicit and not have it match to preroute)
  • move the req$serializer definition into the preserializer step
  • drop post value in the list

The object being returned is being supplied to pr$registerHooks()

@schloerke
Copy link
Collaborator

schloerke commented Jun 10, 2020

Thinking more on your comment about moving it to a true serializer function... I think that's the best solution. But I'd like to revamp parseBlock before doing so.

@meztez
Copy link
Collaborator Author

meztez commented Jun 10, 2020

I've made it work with the current serializer_content_type, let me commit and you can check if it makes senses to you.

@meztez
Copy link
Collaborator Author

meztez commented Jun 10, 2020

Do you want to also unlink the temp file created to store the image during the request processing? As it is, it is not removed.

@meztez
Copy link
Collaborator Author

meztez commented Jun 10, 2020

Ah! I think I understood why the original setup was confusing to me.

Let's

* rename `pre` to `preserializer` (to be explicit and not have it match to `preroute`)

* move the `req$serializer` definition into the `preserializer` step

* drop `post` value in the list

The object being returned is being supplied to pr$registerHooks()

It has to be preroute because the image device has to be turned on to capture output before execSteps execution.

@schloerke
Copy link
Collaborator

schloerke commented Jun 10, 2020

Thank you for the Content-Type updates.

It has to be a preroute

Makes sense. Thank you!

Do you want to also unlink the temp file?

I always like to have a 0-size footprint. This seems like a fair change. I'll consult the team.


  • The return object changed from res to img. Is it possible this value is being used somewhere?

  • Is the data$file accessible anywhere else? This may also help answer if we can delete the file.

@meztez
Copy link
Collaborator Author

meztez commented Jun 11, 2020

it is preexec and postexec, I was mistaken.

@schloerke schloerke requested review from schloerke and cpsievert Jun 11, 2020
R/images.R Outdated Show resolved Hide resolved
R/images.R Outdated Show resolved Hide resolved
R/images.R Outdated Show resolved Hide resolved
@cpsievert cpsievert self-requested a review Jun 11, 2020
Copy link
Contributor

@cpsievert cpsievert left a comment

Approved, pending suggestions

@schloerke schloerke merged commit d80278b into rstudio:master Jun 11, 2020
2 of 3 checks passed
@meztez meztez deleted the image_serializer branch Jun 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: high Must be fixed before next release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants