moodymudskipper / flow

View and Browse Code Using Flow Diagrams
https://moodymudskipper.github.io/flow/
Other
395 stars 26 forks source link

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

Open pvictor opened 12 months ago

pvictor commented 12 months ago

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

moodymudskipper commented 12 months ago

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.

pvictor commented 12 months ago

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.

moodymudskipper commented 12 months ago

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 commented 12 months ago

see also caveat in #157

pvictor commented 12 months ago

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

moodymudskipper commented 11 months ago

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 commented 11 months ago

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

SourceDirectory

moodymudskipper commented 11 months ago

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 ?