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

cl_dl output is missing if string contains # or ' #370

Closed
fmichonneau opened this issue Nov 2, 2021 · 12 comments
Closed

cl_dl output is missing if string contains # or ' #370

fmichonneau opened this issue Nov 2, 2021 · 12 comments
Labels
bug an unexpected problem or unintended behavior
Milestone

Comments

@fmichonneau
Copy link

library(cli)

cli_dl(
  c(
    "test_1" = "all good",
    "test_2" = "not #good"
  )
)
#> test_1: all good
#> test_2:

Created on 2021-11-02 by the reprex package (v2.0.1)

@gaborcsardi gaborcsardi added the bug an unexpected problem or unintended behavior label Nov 2, 2021
@fmichonneau fmichonneau changed the title cl_dl output is missing if string contains # cl_dl output is missing if string contains # or ' Nov 3, 2021
@fmichonneau
Copy link
Author

fmichonneau commented Nov 3, 2021

it looks like ' and " are also causing some trouble:

cli::cli_dl(c("test_3" = "no' good either"))
#> test_3:
cli::cli_dl(c("test_4" = "no\" good also"))
#> test_4:

@zkamvar
Copy link
Contributor

zkamvar commented Nov 10, 2021

This was an issue with {glue}: tidyverse/glue#193 and was fixed in the latest release, but now brings up a new problem:

cli::cli_text("{.url https://example.com/#section}")
#> Error in glue_data(.x = NULL, ..., .sep = .sep, .envir = .envir, .open = .open, : Unterminated comment

Created on 2021-11-10 by the reprex package (v2.0.1)

@zkamvar
Copy link
Contributor

zkamvar commented Nov 10, 2021

This behavior is fixed if you put the strings as variables, however:

url <- "https://example.com/#section"
cli::cli_text("{.url {url}}")
#> <https://example.com/#section>
msg <- "no' good either"
cli::cli_dl(c("test_3" = "{msg}"))  
#> test_3: no' good either

Created on 2021-11-10 by the reprex package (v2.0.1)

This is explicitly tested in

test_that("no glue substitution in expressions that evaluate to a string", {
expect_snapshot(local({
msg <- "Message with special characters like } { }} {{"
cli_text("{msg}")
cli_text("{.emph {msg}}")
}))
})

@jimhester
Copy link
Member

I think probably cli should set comment = character() when calling glue to work around the # issue. For the unpaired quote issues I guess we would need to add an option to disable tracking of quotes when parsing.

@zkamvar
Copy link
Contributor

zkamvar commented Nov 12, 2021

It would certainly help when providing a Klingon translation to messages (in a romanized script):

cli::cli_alert_success("Qapla'")

@gaborcsardi
Copy link
Member

This was fixed by 64a389e, but I messed up the issue number reference....

@gaborcsardi
Copy link
Member

Unfortunately I'll have to re-open this temporarily because it breaks some revdeps, and there is no way to escape braces with .literal, it seems. So until we change that or come up with another solution, the quotes will not work, and you'll need to substitute them in. The comments will be fine, because glue has a separate argument for ignoring them.

@gaborcsardi gaborcsardi reopened this Feb 10, 2022
@gaborcsardi
Copy link
Member

Opened in 5ee4974

@gaborcsardi
Copy link
Member

For the record, AFAICT the problematic cli functions are cli_dl(), cli_process_start() and co. (deprecated) and cli_progress_step(), because they stitch on classes to the user input. This is definitely an anti-pattern, and I'll try to avoid it. That improves the situation because people only need to worry about quotes if they use them within braces.

❯ cli_dl(c(x = "foobar'"))
Error in glue_data(.x = NULL, ..., .sep = .sep, .envir = .envir, .open = .open,  :
  Unterminated quote (')

❯ cli_process_start("foobar'")
Error in glue_data(.x = NULL, ..., .sep = .sep, .envir = .envir, .open = .open,  :
  Unterminated quote (')

❯ cli_progress_step("foobar'")
Error in glue_data(.x = NULL, ..., .sep = .sep, .envir = .envir, .open = .open,  :
  Unterminated quote (')

@jennybc
Copy link
Member

jennybc commented May 20, 2022

I have just been re-reminded of this issue.

Currently googledrive's basic messaging about files won't work if the name contains, e.g., a single quote. Which is a fairly common phenomenon.

The way I call cli::cli_bullets() is a bit more complicated than this, but boils down to this:

x <- c(
  "Drive file:",
  "*" = "{.file boring_file_name} {cli::col_grey('<id: 1_CqHhC>')}"
)
cli::cli_bullets(x)
#> Drive file:
#> • 'boring_file_name' <id: 1_CqHhC>

x <- c(
  "Drive file:",
  "*" = "{.file I'm a challenging file name} {cli::col_grey('<id: 1_CqHhC>')}"
)
cli::cli_bullets(x)
#> Error in glue_data(.x = NULL, ..., .sep = .sep, .envir = .envir, .open = .open, : Unterminated quote (')

(In real life, I have written a custom inline style for Google Drive file names, instead of .file, but we can see the problem with .file.)

I'm not even quite sure where I could insert .literal = TRUE (in the glue sense) into my cli workflows / calls to fix this situation.

@gaborcsardi
Copy link
Member

The pattern that works is

fn <- "I'm a challenging file name"
x <- c(
  "Drive file:",
  "*" = "{.file {fn}} {cli::col_grey('<id: 1_CqHhC>')}"
)
cli::cli_bullets(x)

@gaborcsardi
Copy link
Member

In general, to fix this cli needs its own parser, because the current cli extensions cannot be parsed properly with multiple rounds of glue with some combination of .literal = TRUE and .literal = FALSE.

gaborcsardi added a commit that referenced this issue Aug 18, 2022
jgires added a commit to phacochr/phacochr that referenced this issue Oct 13, 2022
- Désactivation des cli_progress_step, qui causent TOUS une erreur chez moi. Le pb est connu : r-lib/cli#370
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug an unexpected problem or unintended behavior
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants