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

Parsing for % characters #879

Closed
ha0ye opened this issue Jul 18, 2019 · 7 comments · Fixed by #904
Closed

Parsing for % characters #879

ha0ye opened this issue Jul 18, 2019 · 7 comments · Fixed by #904

Comments

@ha0ye
Copy link

@ha0ye ha0ye commented Jul 18, 2019

Can roxygen include a warning when the resulting .Rd file is mis-formatted as a result of un-escaped % characters? (which end up as comments in the .Rd format)

This sometimes produces a warning for mismatched quotes or braces (e.g. #530, #742, #786), but this doesn't always seem to be caught until R CMD CHECK.

reprex below:

library(roxygen2)

lines <- c("#' compute a percentage",
           "#' @param x the % value to use",
           "#' @export",
           "f <- function(x, y) {x / 100 * y}")

roc_proc_text(rd_roclet(), lines)
#> $f.Rd
#> % Generated by roxygen2: do not edit by hand
#> % Please edit documentation in RtmpcTEw4i/file6ca12af04873
#> \name{f}
#> \alias{f}
#> \title{compute a percentage}
#> \usage{
#> f(x, y)
#> }
#> \arguments{
#>   \item{x}{the % value to use}
#> }
#> \description{
#> compute a percentage
#> }

Created on 2019-07-18 by the reprex package (v0.3.0)

@hadley
Copy link
Member

@hadley hadley commented Jul 21, 2019

I'm not sure if we'll be able to catch this one easily, but maybe we can check for an unescaped % on the final line.

@hadley
Copy link
Member

@hadley hadley commented Jul 25, 2019

The check is done in rdComplete() — it would probably need an additional argument as to whether or not it's a multi-line block. If that argument was true, and in_latex_comment was true, then roxygen_parse_tag() would set complete to false

@hadley hadley added this to the v6.2.0 milestone Aug 22, 2019
@hadley
Copy link
Member

@hadley hadley commented Sep 10, 2019

The check shouldn't be on whether or not it's an multiline block, but whether or not the output is going to generate Rd. And rdComplete() doesn't know anything about code blocks, so my initial idea won't work at all.

Maybe it's just better to automatically escape % while doing the markdown processing? The downside is we'll have to figure out how to handle existing \% (which will be converted to \\%) but maybe that's rare enough that a one time break is worth it.

hadley added a commit that referenced this issue Sep 10, 2019
@hadley hadley closed this in #904 Sep 12, 2019
hadley added a commit that referenced this issue Sep 12, 2019
Providing detailed message when you upgrade

Fixes #879
@tbates
Copy link
Contributor

@tbates tbates commented Sep 18, 2019

Be nice now if the inverse of this comment was true: "Can roxygen include a warning when the resulting .Rd file is mis-formatted as a result of escaped % characters? (which end up as backslashes in the .Rd format)

@gavinsimpson
Copy link

@gavinsimpson gavinsimpson commented Jan 14, 2020

Ugggh; this change to escape % but not check if it's already escaped is going to bite people in the ass a lot; This caught me out at the weekend (\% -> \\%) but the errors I was seeing upon converting to roxygen to Rd weren't at all helpful in tracking down what was actually wrong.

Any chance that this could be revisited so that \% isn't automagically escaped again? I'll open a new issue if you;d like but thought best to ask here first.

@trashbirdecology
Copy link

@trashbirdecology trashbirdecology commented Jan 14, 2020

Ugggh; this change to escape % but not check if it's already escaped is going to bite people in the ass a lot; This caught me out at the weekend (\% -> \\%) but the errors I was seeing upon converting to roxygen to Rd we're at all helpful in tracking down what was actually wrong.

Any chance that this could be revisited so that \% isn't automagically escaped again? I'll open a new issue if you;d like but thought best to ask here first.

@gavinsimpson update "we're" to "were not(?)"

@hadley
Copy link
Member

@hadley hadley commented Jan 14, 2020

Unfortunately, I don't think changing the policy will help; it'll just add more confusion. Having to escape % sucks, but I'm reasonably confident it's the net least painful solution long term.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

5 participants