Restore old behavior of @importFrom in some edge cases #1574
Conversation
|
This bug also affects |
| warn_roxy_tag(x, "Excluding unknown {cli::qty(sum(unknown_idx))} export{?s} from {.package {pkg}}: {.code {importing[unknown_idx]}}") | ||
| if (all(unknown_idx)) { | ||
| return(NULL) | ||
| } | ||
| x$val <- c(pkg, importing[!unknown_idx]) | ||
| } | ||
| } |
There was a problem hiding this comment.
One more thought: how do the different types of quoting affect the NAMESPACE? Are they normalised elsewhere, or is it ok to use any of the forms without quoting?
There was a problem hiding this comment.
Do you mean from roxygen2 perspective or base R?
Answering for the latter: NAMESPACE needs to parse as R, so importFrom(magrittr, %>%) won't work.
So for roxygen2, my understanding is @importFrom magrittr %>% will add quotes for convenience (that's the helper auto_quotes), but quoted forms are dropped in as-is.
|
@jdblischak do you see something systematically different in that use case to the magrittr example? Or do you think we should just make the news bullet a bit more clear that this applies to any non-syntactic function name? |
Exactly. From this PR and the linked Issue, it gives the impression that the edge case is specific to the magrittr pipe. Also, I just learned that |
|
@jdblischak thanks for the feedback, it's a great point we can use this bullet to remind readers of available convenience features. PTAL if the current bullet gets the point across. |
NEWS.md
Outdated
| @@ -2,6 +2,12 @@ | |||
|
|
|||
| * `@family` lists are now ordered more carefully, "foo1" comes after "foo" (#1563, @krlmlr). | |||
|
|
|||
| * `@importFrom magrittr "%>%"` works again, as does | |||
| ``@importFrom rlang `:=` `` (#1570, @MichaelChirico). The form | |||
There was a problem hiding this comment.
I think I'd start the bullet with something like `` @importFrom with quoted non-syntactic functions works again, e.g. ...`.
There was a problem hiding this comment.
Agreed. I like the explicit examples, but I think there could be more verbiage to explain what happened. Something like the following:
* `@importFrom` with quoted non-syntactic functions works again, e.g.
`@importFrom magrittr "%>%"` and ``@importFrom rlang `:=` `` (#1574,
@MichaelChirico). This was accidentally broken only in 7.3.0 (#1570). The form
with an unquoted non-syntactic function, e.g. `@importFrom magrittr %>%`,
has continued to work, even in 7.3.0. Relatedly, `@importFrom` directives not
matching any known functions (e.g. `@importFrom utils plot pdf`) will not
produce invalid NAMESPACE files.
There was a problem hiding this comment.
Thanks for the feedback, PTAL.
|
|
||
| import <- expect_no_warning(roc_proc_text(namespace_roclet(), lines)) | ||
| expect_equal(import, c( | ||
| "importFrom(stringr,\"%>%\")", |
There was a problem hiding this comment.
for my edification, any reason to prefer this to
| "importFrom(stringr,\"%>%\")", | |
| 'importFrom(stringr,"%>%")', |
I like this for keeping the 3 lines of code aligned vertically / no need to visually parse escapes.
There was a problem hiding this comment.
I don't have particularly strong feelings here but I like that currently all the strings use ", and are consistent with the values in lines. I saw you used ' for the input "%>%", but I didn't like having to escape #'.
Closes #1570