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

Code in markdown link text omits closing } #620

Closed
egnha opened this issue Apr 10, 2017 · 11 comments
Closed

Code in markdown link text omits closing } #620

egnha opened this issue Apr 10, 2017 · 11 comments

Comments

@egnha
Copy link
Contributor

@egnha egnha commented Apr 10, 2017

In roxygen2 6.0.1, specifying code in markdown link text fails to generate a properly enclosed \code tag.

Example: The block

#' foo
#' @param x Leaky scope [`tryCatch`][base::tryCatch()].
foo <- function(x) NULL

produces an incomplete Rd file:

% Generated by roxygen2: do not edit by hand
% Please edit documentation in R/foo.R
\name{foo}
\alias{foo}
\title{foo}
\usage{
foo(x)
}
\arguments{
\item{x}{Leaky scope \code{\link[base:tryCatch]{tryCatch}.}
}
\description{
foo
}

\code omits a closing }.

Is this related to #521?

@egnha egnha changed the title Code in markdown link text not properly enclosed Code in markdown link text omits closing } Apr 10, 2017
@gaborcsardi
Copy link
Collaborator

@gaborcsardi gaborcsardi commented Apr 12, 2017

Yes, this looks like a bug. Here is an example that you can run easily:

roc <- rd_roclet()
out1 <- roc_proc_text(roc, "
  #' foo
  #' @md
  #' @param x Leaky scope [`tryCatch`][base::tryCatch()].
  foo <- function(x) NULL")[[1]]
out1
#> % Generated by roxygen2: do not edit by hand
#> % Please edit documentation in RtmpJ0GHaB/file1e2e4ccca13e
#> \name{foo}
#> \alias{foo}
#> \title{foo}
#> \usage{
#> foo(x)
#> }
#> \arguments{
#>   \item{x}{Leaky scope \code{\link[base:tryCatch]{tryCatch}.}
#> }
#> \description{
#> foo
#> }

@egnha
Copy link
Contributor Author

@egnha egnha commented Apr 13, 2017

A variation on the above example:

block <- "
  #' foo
  #' @md
  #' @param x Leaky scope [%s][stats::filter()].
  foo <- function(x) NULL
"
rd_link <- function(md) roc_proc_text(rd_roclet(), sprintf(block, md))[[1L]]
with_link <- lapply(setNames(nm = c("`filter`", "filter()")), rd_link)

with_link[["`filter`"]] is invalid, as before, but with_link[["filter()"]] produces properly scoped Rd, as expected.

However, in keeping with the the convention that ...() implies that the link text is code, would it be more consistent if both outputs of with_link were to produce \code{}-wrapped links, i.e., \code{\link[stats:filter]{link_text}}?

@egnha
Copy link
Contributor Author

@egnha egnha commented Apr 13, 2017

Looks like this is the offending line in markdown.R. The omission can be rectified with if (is_code) "}}" else "}". (For consistency, the corresponding lines in the if clause could be similarly replaced.)

Shall I submit a PR (with tests)?

egnha added a commit to egnha/roxygen that referenced this issue Apr 13, 2017
@gaborcsardi
Copy link
Collaborator

@gaborcsardi gaborcsardi commented Apr 13, 2017

I am pretty sure that this is incorrect, and will probably also break a bunch of tests.

@egnha
Copy link
Contributor Author

@egnha egnha commented Apr 13, 2017

If you are referring to my suggestion that with_link[["filter()"]] should be \code-wrapped as well, then I agree: not a good idea, as it'd needlessly introduce exceptional behavior that is adequately taken care of by backticks.

As for the one-line change, I tried it out against current master, and all existing tests passed. (Can you confirm this?) Of course, new tests, and an additional line to the dummy page markdown-test, should be added to confirm the fix.

@gaborcsardi
Copy link
Collaborator

@gaborcsardi gaborcsardi commented Apr 13, 2017

Oh, you are right, I misread your patch first. Pls add a test, a bullet point to the NEWS and submit a PR. Thanks much!

@gaborcsardi
Copy link
Collaborator

@gaborcsardi gaborcsardi commented Apr 13, 2017

Btw. the first part of the patch is not needed imo, and pls write the second part as

  "}",
  if (is_code) "}"		

Then you just add a single line, and it is clear what the bug was.

@egnha
Copy link
Contributor Author

@egnha egnha commented Apr 13, 2017

Will be glad to submit a PR.

I will leave the if-clause as is. But changing the second part as you suggested causes breakages (because you'll get a trailing NULL in the list-output of parse_link() when is_code is FALSE); in particular, a number of assertions in "short and sweet links work" would fail (test-rd-markdown-links.R).

@gaborcsardi
Copy link
Collaborator

@gaborcsardi gaborcsardi commented Apr 13, 2017

paste0 ignores NULLs, so if that causes an error, then I guess sg else is wrong.

❯ paste0("a", "b")
[1] "ab"

❯ paste0("a", "b", NULL)
[1] "ab"

@egnha
Copy link
Contributor Author

@egnha egnha commented Apr 13, 2017

You're right, something else is wrong. I believe the problem is markdown_rparse(): It has no case-clause to handle the NULL class, so if parse_link() returns a list containing a NULL, then markdown_rparse() will recurse down to a NULL base case, which it unspecified, so the warning "Unknown xml object" is signaled without the proper value being returned, namely the empty string.

So to make the patch as you've suggested (add line rather than change line), the clause

  if (is.null(xml)) {
    return("")  # Alternatively, return(character(1))
  }

should be added to markdown_rparse(), before warning("Unknown xml object") can be invoked. Then the bug is fixed in the way you've suggested without spoiling the existing tests.

@egnha
Copy link
Contributor Author

@egnha egnha commented Apr 13, 2017

Actually, I think it would be simplest to just have

if (is_code) "}" else ""

at the end of each clause of the final if-else of parse_link(). Then it is clear what is changed and why, type consistency is maintained (the root cause of the test failures), and no change would be required in markdown_rparse(), or elsewhere.

egnha added a commit to egnha/roxygen that referenced this issue Apr 14, 2017
egnha added a commit to egnha/roxygen that referenced this issue Apr 14, 2017
egnha added a commit to egnha/roxygen that referenced this issue Apr 14, 2017
@hadley hadley closed this in 35050ae Aug 16, 2017
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.

None yet
3 participants