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

Comment inside {} suppresses output #224

Closed
gaborcsardi opened this issue Sep 13, 2021 · 5 comments · Fixed by #227
Closed

Comment inside {} suppresses output #224

gaborcsardi opened this issue Sep 13, 2021 · 5 comments · Fixed by #227
Labels
bug an unexpected problem or unintended behavior

Comments

@gaborcsardi
Copy link
Member

I realize this is an edge case, and you probably would not write code like this, but it may happen if the code within {} is machine-generated:

c(glue::glue("pre {1+5 # never mind me} post"))
#> [1] "pre "

It is not entirely straightforward what the output should be, maybe the interpolation should end at }, and it then it would be "pre 6 post"?

@jimhester
Copy link
Collaborator

jimhester commented Sep 13, 2021

This seems to be a case that is not properly handled, the code assumes comment lines will always end with a newline.

e.g. this works

glue::glue("pre {1+5 # never mind me
} post")
#> pre 6 post

Created on 2021-09-13 by the reprex package (v2.0.0)

@jimhester jimhester added the bug an unexpected problem or unintended behavior label Sep 13, 2021
@gaborcsardi
Copy link
Member Author

That makes sense. Doe you think it would make sense to "fix" this for the case without a newline? Would you review a PR for this?

@jimhester
Copy link
Collaborator

yeah

@gaborcsardi
Copy link
Member Author

gaborcsardi commented Sep 25, 2021

Now I am not sure anymore how to "fix" this. E.g. this seems fine, and we probably don't want to look for closing delims eagerly:

glue::glue("pre {1 + 5 + # never mind the }\n 4} post")
pre 10 post

OTOH the original example surely does not make sense, so maybe that should be an error? I.e. if the string ends in comment state, that could be an error?

gaborcsardi added a commit that referenced this issue Sep 25, 2021
Instead of generating an empty string.

Closes #224.
@jimhester
Copy link
Collaborator

Yeah, I think if we end in a comment state making that an error makes sense to me

Robsteranium added a commit to Robsteranium/csvwr that referenced this issue Nov 8, 2021
The URI template spec supports [fragment identifiers](https://datatracker.ietf.org/doc/html/rfc6570#section-3.2.4)
by expanding a template like `{#var}`. Glue interprets the hash char as a comment as it's inside curly braces.
[A recent change to glue](tidyverse/glue#224) flushed this out.

This patch rewrites the template as `#{var}`. This has the desired effect when the value is present but doesn't
conform with the spec properly as the whole fragment (including the `#`) should be omitted. This is raised in #4.
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.

2 participants