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

Fully use glue package #155

Closed
echasnovski opened this Issue Jul 23, 2018 · 8 comments

Comments

Projects
None yet
2 participants
@echasnovski
Copy link
Collaborator

echasnovski commented Jul 23, 2018

This is an issue for my planned PR. It will be at least about:

  • Replace all stop(paste(...)), warning(paste(...)), and message(paste(...)) with stop_glue(...), warning_glue(...), and message_glue(...). @ismayc , should those be replaced in commented code chunks (like this one)?
  • Replace all paste(...) (using data) with glue::glue(...).

No code logic is expected to be affected.

@ismayc

This comment has been minimized.

Copy link
Collaborator

ismayc commented Jul 23, 2018

@echasnovski Yes, I think that would be helpful as those comments may or may not be included at a later date, so it would be helpful to have them standardized. This should really help with code readability. Thanks much!

@echasnovski

This comment has been minimized.

Copy link
Collaborator Author

echasnovski commented Jul 24, 2018

After playing with {glue} I've found this unexpected behavior:

library(glue)

world <- NULL

# Returns empty string
str(glue::glue("Hello {world}!"))
#> Classes 'glue', 'character'  chr(0)

# Treats `NULL` as `character(0)`
paste0("Hello ", world, "!")
#> [1] "Hello !"

For this issue it means that if any object inside "{}" evaluates into NULL then no text in error (warning, message) will be given. I don't think this is crucial because printing empty string to indicate NULL is generally a bad idea. A better way to do this is give extra notification if object is NULL when it shouldn't be.
After studying code I found no places where this might be crucial (as checks for not being NULL are made beforehand), but this is something to keep in mind. In #156 and #157 there are no checks but errors occur earlier.

@echasnovski

This comment has been minimized.

Copy link
Collaborator Author

echasnovski commented Jul 24, 2018

Unfortunately, it doesn't seem to:

library(glue)

world <- NULL

str(glue::glue("Hello {world}!", .na = ""))
#> Classes 'glue', 'character'  chr(0)
str(glue::glue("Hello {world}!", .na = NULL))
#> Classes 'glue', 'character'  chr(0)
str(glue::glue("Hello {world}!", .na = "   "))
#> Classes 'glue', 'character'  chr(0)

It is said that .na deals with NA values, not NULL.

@ismayc

This comment has been minimized.

Copy link
Collaborator

ismayc commented Jul 24, 2018

Here we go I think: tidyverse/glue#45

@ismayc

This comment has been minimized.

Copy link
Collaborator

ismayc commented Jul 24, 2018

Haha. You are one step ahead of me. Never mind :)

@echasnovski

This comment has been minimized.

Copy link
Collaborator Author

echasnovski commented Jul 24, 2018

However, slide about transformers helps:

library(glue)

null_transformer <- function(text, envir) {
  out <- identity_transformer(text, envir)
  if (is.null(out)) {
    return("NULL")
  }
  
  out
}

world <- NULL

glue("Hello {world}!", .transformer = null_transformer)
#> Hello NULL!

It might be a good idea to use this. @ismayc, what do you think?

@ismayc

This comment has been minimized.

Copy link
Collaborator

ismayc commented Jul 24, 2018

I think I like this too.

andrewpbray added a commit that referenced this issue Jul 27, 2018

Merge pull request #158 from echasnovski/glue
Fully use glue package (#155)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment