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

change envir eval for plumb block #620

Closed
wants to merge 3 commits into from

Conversation

meztez
Copy link
Collaborator

@meztez meztez commented Jul 28, 2020

Fixes #619

PR task list:

  • [NA] Update NEWS

  • Add tests

  • [NA] Update documentation with devtools::document()

  • Do to the same for parsers once merged.

@meztez
Copy link
Collaborator Author

meztez commented Jul 28, 2020

One thing I noticed is the treatment for the image arguments are evaluated in evaluateBlock instead of plumbBlock. It also add list in front of the arguments.

Could we move the eval to plumbBlock so we get a narrower stopOnLine message in case of errors. We could also make list optional by appending it to evals for serializers and parsers (in the other PR)

@schloerke
Copy link
Collaborator

schloerke commented Jul 28, 2020

Could we move the eval to plumbBlock?

Yes, please! (Feel free to do that in this PR.)



The parsing is inconsistent.

Correct.

List of #' @KIND TYPE list(ARGS) setups and their equivalent shorthand

#' @parser json list(auto_unbox = TRUE)
## no short hand form of @parser

#' @serializer html list()
#' @serializer html
## shorthand for html specifically
#' @html()
#' @html

#' @serializer json list(auto_unbox = TRUE)
## shorthand for json specifically
#' @json(auto_unbox = TRUE)
#' @json

## No long hand form of images
#' @jpeg
#' @jpeg(w=1)

Should we have a separate PR for image serializers? (Will be MUCH easier once #584 is merged)

Something like...

register_image_serializer = function(name, content_type, dev_on, dev_off = grDevices::dev.off, verbose = TRUE) {
  .... # check for existing and warn if verbose
  .globals$image_serializers[[name]] <- list(
    content_type = content_type,
    dev_on = dev_on,
    dev_off = dev_off
  )
}
  • registered_image_serializer()
  • Export image_png, image_jpeg, image_svg
  • Could maybe have an exported ep_image(endpoint, image_name) method to facilitate registering the hooks and serializer

Then the situation brings up how should we handle image serializers? Currently, there is only #' @IMAGE(arg1 = val1, ..). Should we do #' @image jpeg list(arg1 = val1, ..)?

Does the list text seem redundant? Should we allow for #' @KIND TYPE(ARGS) rather than only #' @KIND TYPE list(ARGS). Ex: #' @image jpeg(arg1 = val1, ..) or #' @parser json(simplifyVector = TRUE)

@meztez
Copy link
Collaborator Author

meztez commented Jul 28, 2020

I'll move the eval in this PR. Will do a separate PR for plumber syntax once #584 merged.

@schloerke
Copy link
Collaborator

schloerke commented Jul 28, 2020

Thank you!


Does the proposed image make sense to implement? ...Seems to follow pattern of the serializers and parsers

@meztez
Copy link
Collaborator Author

meztez commented Jul 28, 2020

Yes I think the list is redundant, unsure about the parentheses, I quite like them. But the plumbing block need a tidbit more love, between the shortname for serializers, it is a bit hard to follow.

@schloerke
Copy link
Collaborator

schloerke commented Jul 28, 2020

unsure about the parentheses, I quite like them

Yes, let's keep the parentheses.

between the shortname for serializers, it is a bit hard to follow

Yes, I'd like to remove all examples of @json and @html in the docs. IDK if we should soft deprecate it or just leave it there, hidden.

Copy link
Collaborator

@schloerke schloerke left a comment

(Should wait until other PRs are merged) LGTM!

@schloerke schloerke requested a review from cpsievert Jul 28, 2020
@meztez
Copy link
Collaborator Author

meztez commented Jul 28, 2020

I'm thinking about generalizing block parsing syntax so we have a list of block regex instead of whole blobby mess that it is at the moment. I think we already discussed it.

@schloerke
Copy link
Collaborator

schloerke commented Jul 28, 2020

That'd be great. Would be much easier to read. (Don't know how the variables will be handled)


If you're looking for plumber enhancements, I'd gladly take the removal of Digital Ocean code to an external package. (#593) ... would rather not keep those functions if I can help it. 😬😄

@meztez
Copy link
Collaborator Author

meztez commented Jul 28, 2020

I thought I had a PR for digital ocean stuff. Maybe I forgot to push.

@cpsievert
Copy link
Contributor

cpsievert commented Jul 29, 2020

Generally agree with #620 (comment). Maybe the naming should be register_device(), device_*() (for png, jpeg, etc), and #' @device png (width=640)

cpsievert added a commit that referenced this pull request Jul 29, 2020
Co-authored-by: Bruno Tremblay <bruno.tremblay@lacapitale.com>
@schloerke
Copy link
Collaborator

schloerke commented Jul 31, 2020

This code is being merged in #623. (Added a news entry)

Closing here.

@schloerke schloerke closed this Jul 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Plumber comments should be evaluated in the correct environment
3 participants