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

Inconsistent detection of the magrittr dot (.) as an NSE variable #26

Open
wlandau opened this issue Mar 13, 2018 · 3 comments
Open

Inconsistent detection of the magrittr dot (.) as an NSE variable #26

wlandau opened this issue Mar 13, 2018 · 3 comments

Comments

@wlandau
Copy link

wlandau commented Mar 13, 2018

From ropensci/drake#320:

x <- CodeDepends::getInputs(quote(read_csv("data.csv") %>% mutate(.data = ., 
  foo = bar + baz)))
#> Warning: replacing previous import 'graph::addNode' by 'XML::addNode' when
#> loading 'CodeDepends'
#> Warning: replacing previous import 'graph::plot' by 'graphics::plot' when
#> loading 'CodeDepends'
x@inputs
#> character(0)
x@nsevalVars
#> [1] "."   "bar" "baz"

x <- CodeDepends::getInputs(quote(raw_data %>% select(., starts_with("xyz"))))
x@inputs
#> [1] "raw_data"
x@nsevalVars
#> [1] "."

x <- CodeDepends::getInputs(quote(raw_data %>% filter(.)))
x@inputs
#> [1] "."        "raw_data"
x@nsevalVars
#> character(0)

x <- CodeDepends::getInputs(quote(raw_data %>% filter(complete.cases(.))))
x@inputs
#> [1] "."        "raw_data"
x@nsevalVars
#> character(0)
@gmbecker
Copy link
Collaborator

@wlandau

So filter has different semantics whether you are hitting the Bioconductor or dplyr version. The way that CodeDepends currently works (which is not quite perfectly correct but would cover the case here, I suspect) is that if dplyr is loaded previously in the script CodeDepends is aware of, it handles arguments to filter the way you would want for dplyr::filter. If not, it treats the filter call normally.

i.e.,

> expr = readScript(txt = "library(dplyr); raw_data %>% filter(.)")
> getInputs(expr)[[2]]@inputs
[1] "raw_data"
> getInputs(expr)[[2]]@nsevalVars
[1] "."

You're probably right that more generally across everything we might want . to always be marked as nse (or ignored entirely, to be honest). I can look at doing that but that will take a bit more doing.

How is the code getting into CodeDepends? Is drake tracking which libraries are loaded somehow, or does it have a full script to operate on rather than only individual expressions (even beyond this, just in general CodeDepends is going to behave better and be more powerful/correct when operating on the whole script).

@wlandau
Copy link
Author

wlandau commented Mar 14, 2018

Contrary to the established traditions of Make-like tools, drake focuses on the user's R session, not script files. It analyzes the commands in the workflow plan data frame and the "imported" functions loaded in your R session (or a custom environment you provide). If CodeDepends behaves differently depending on which packages are loaded, targets could be invalidated in unpredictable ways, which does not bode well for drake.

@gmbecker
Copy link
Collaborator

gmbecker commented Mar 14, 2018 via email

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

No branches or pull requests

2 participants