-
Notifications
You must be signed in to change notification settings - Fork 235
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
Throw an error for @importFrom unknown export #1458
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems reasonable to me.
I can't make snapshots from my current machine, so I'll try and pull them from the GHA artifacts. Should be ready for review anyway. |
Thanks! |
pkg <- x$val[1L] | ||
if (requireNamespace(pkg, quietly = TRUE)) { | ||
importing <- x$val[-1L] | ||
unknown_idx <- !importing %in% getNamespaceExports(pkg) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
two issues with this PR in retrospect as surfaced by #1570:
importing
can be like"%>%"
or`%>%`
or'%>%'
butgetNamespaceExports()
will always just be the plain symbol (%>%
)importing[!unknown_idx]
might drop everything --> output becomesimportFrom($PKG,)
How would you recommend approaching these?
- Is a bit tricky because we have to normalize the quotes, I guess we can do
gsub("^(`|'|\")(.*)\\1$", "\\1", importing)
. This could be a helper alongsidehas_quotes()
Initially, I was worried about collision where you try and import a really weirdly-named object likeas.name("'%>%'")
, but I guess {roxygen2} has always made that difficult. And a quick search doesn't turn up any usage of this anyway: https://github.com/search?q=lang%3AR+%2F%5Cn%5Cs*%28%22%27.*%27%22%7C%27%22.*%22%27%7C%60%22.*%22%60%7C%60%27.*%27%60%7C%22%60.*%60%22%7C%27%60.*%60%27%29%5Cs*%3C-%2F+-path%3A.Rd&type=code - Should this method just
return(NULL)
ifall(unknown_idx)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Yeah, I think
strip_quotes()
seems reasonable. - Yes, I think so.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks! that's basically how I did #1574 so let's continue there.
Closes #1409.
This is a rough / off-the-cuff version. Wanted to checkpoint here to get high-level feedback, there's still TODO:
cli
)