flow icon indicating copy to clipboard operation
flow copied to clipboard

FR: flow_view_source_calls: watch for R.utils::sourceDirectory

Open pvictor opened this issue 2 years ago • 8 comments

Hello,

A little feature request : it could be interesting to included directories of scripts sourced with sourceDirectory, or even better declare a list of sourcing calls to watch for in an argument of the function.

Victor

pvictor avatar Jul 04 '23 09:07 pvictor

Hi @pvictor , thanks for the FR. Can you show me an example of what you have in mind when you say "declare a list of sourcing calls to watch for in an argument of the function" ?

sourceDirectory() is not a base function but it seems it wouldn't be too hard to support at least partially, that means I would ignore the pattern and recursive args if fed as a variable and not a literal string (since it's static analysis). I would support SourceTo() from the same package for consistency.

moodymudskipper avatar Jul 04 '23 10:07 moodymudskipper

I was thinking of something like this (sorry, it was much clearer in my head in french) :

flow::flow_view_source_calls(
  paths = ".",
  more_calls = c("sourceDirectory", "R.utils::sourceDirectory")
)

Where the rule is that only the first argument is checked.

pvictor avatar Jul 05 '23 07:07 pvictor

I think I'd rather use the recursive and pattern args if possible and and I think false positives are unlikely if I just support it out of the box.

I've drafted it, can you update and try it ?

It's not documented nor formally tested yet, and I should probably include testthat::source_file() and testthat::source_dir() as well.

moodymudskipper avatar Jul 05 '23 09:07 moodymudskipper

see also caveat in #157

moodymudskipper avatar Jul 05 '23 09:07 moodymudskipper

Thanks! I have an error with GitHub version :

Erreur dans (function (..., row.names = NULL, check.rows = FALSE, check.names = TRUE,  : 
  les arguments impliquent des nombres de lignes différents : 5, 0

Here an example repo with the code I ran : https://github.com/pvictor/flow-source-calls

pvictor avatar Jul 06 '23 13:07 pvictor

Great reprex, thanks a lot for the effort!

Interesting case also, you have in app/global.R :

R.utils::sourceDirectory("funs/")
R.utils::sourceDirectory("modules/")

Seemingly referring to the funs and modules subfolder, but the working directory is not decided by the file being run, so "funs" will refer to the folder under the root, and "modules" doesn't exist, so has zero eligible files and this corner case is not gracefully handled.

moodymudskipper avatar Jul 07 '23 10:07 moodymudskipper

In fact R.utils::sourceDirectory() seems to have some erratic behavior, can we trust it ? Can you explain this behavior @pvictor ?

SourceDirectory

moodymudskipper avatar Jul 07 '23 12:07 moodymudskipper

I've pushed changes so that the reported bug is fixed (considering changes of working directory), the behavior above worries me though, R.utils::sourceDirectory() doesn't fail if the folder doesn't exist and it seems to look into a different place (relative vs absolute) when the file was just changed, despite wrapping sourceTo with chdir = FALSE. That doesn't make much sense to me but these silent errors are dangerous. While solving the issue I also had this strange thing happen where my working directory was set to "/", not sure how.

@HenrikBengtsson do you know what might happen above ?

moodymudskipper avatar Jul 07 '23 12:07 moodymudskipper