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

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

Open
pvictor opened this issue Jul 4, 2023 · 8 comments
Open

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

pvictor opened this issue Jul 4, 2023 · 8 comments
Milestone

Comments

@pvictor
Copy link

pvictor commented Jul 4, 2023

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
Copy link
Owner

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 moodymudskipper added this to the 0.3.0 milestone Jul 4, 2023
@pvictor
Copy link
Author

pvictor commented Jul 5, 2023

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
Copy link
Owner

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
Copy link
Owner

see also caveat in #157

@pvictor
Copy link
Author

pvictor commented Jul 6, 2023

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
Copy link
Owner

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
Copy link
Owner

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

SourceDirectory

@moodymudskipper
Copy link
Owner

moodymudskipper commented Jul 7, 2023

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 ?

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