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

[LINT_BUG]: Error when searching for module which uses ../ #137

Open
tyner opened this issue Aug 21, 2024 · 3 comments
Open

[LINT_BUG]: Error when searching for module which uses ../ #137

tyner opened this issue Aug 21, 2024 · 3 comments

Comments

@tyner
Copy link

tyner commented Aug 21, 2024

box.linters version

0.10.2

Sample source code to lint

Somewhat similar to #110 but this time using ../

Create a file Foo.R containing:

foo <- function(x) {
    box::use(../Bar[baz])

    baz(x)
}

Then in the parent directory of the directory where Foo.R lives, create a file Bar.R containing:

baz <- function(x) {
    message(x, " from ", sys.call()[[1L]])
}

(NB: I omitted # 'export directives above because those were not playing nice with Markdown).

then run the following to verify that the example is a valid setup from the perspective of box mechanics

options(box.path = "/path/to/directory") # adjust this to your actual test directory
box::use(./Foo[foo])
foo("hello")

(it prints out hello from baz as a confirmation)

Lint command used

lintr::lint(filename = "/path/to/directory/Foo.R", # adjust this to your actual test directory
            linters = box.linters::box_default_linters
            )

I tried various strategies to get it to work, including setting options(box.path) as well as the working directory.

Lint result

Error: Linter 'box_mod_fun_exists_linter' failed in /path/to/directory/Foo.R: unable to load module “../Bar”; not found

Expected result

Would be nice for it to find the module. Perhaps this is not supported, but I didn't see it mentioned in the documentation.

@TymekDev
Copy link

@tyner I have run into—what I thought—was a similar issue and decided do reproduce your issue. However, I couldn't quite do that.

Setup

I created the following structure:

.
├── directory
│   └── Foo.R
├── Bar.R
├── lint.R
└── main.R

I added #' @export before functions in Foo.R and Bar.R. Otherwise, they are the same as in your comment.

lint.R

lintr::lint("directory/Foo.R", linters = box.linters::box_default_linters)

main.R

options(box.path = "directory")
box::use(./Foo[foo])
foo("hello")

I am using R 4.2.2 and {box.linters} installed from source 7b25817 (the v0.10.2 tag is missing).

Outcome

Running lint.R doesn't produce any lints. Running main.R errors on import.

CleanShot.2024-11-13.at.09.49.28.mp4

I played with this a bit more to get a better understanding and I think this is the expected behavior.

A Search Path section in box::use's documentation confirms this (emphasis is mine):

Modules are searched in the module search path, given by getOption('box.path').
[...]
Local import declarations (that is, module prefixes that start with ./ or ../) never use the search path to find the module. Instead, only the current module’s directory (for ./) or the parent module’s directory (for ../) is looked at.

When to use box.path

In the gist, box.path should be used if you wanted to have a Baz.R script in directory/ that would use an absolute import. For example:

directory/Baz.R

box::use(
  directory/Foo[foo]
)

Without setting the box.path to the root directory (the directory/'s parent), you would get a following error:

Error in box::use(directory/Baz) :
  unable to load module “directory/Foo”; not found in “~/Library/R/arm64/4.2/library/box/mod”, “~/work/playground/box.linters/issue-137/directory”
Calls: <Anonymous> ... tryCatchList -> tryCatchOne -> <Anonymous> -> rethrow
Execution halted

@TymekDev
Copy link

There is an issue about mixing box.path with relative imports, though.

Setup

.
├── directory
│   ├── foo.R
│   └── main.R
├── lint.R
└── lint_box_path.R

directory/foo.R

#' @export
hello <- function() {
  print("hello world!")
}

directory/main.R

box::use(
  ./foo[hello]
)

hello()

lint.R

lintr::lint("directory/main.R", linters = box.linters::box_default_linters)

lint_box_path.R

options(box.path = getwd())
lintr::lint("directory/main.R", linters = box.linters::box_default_linters)

Outcome

  1. Rscript directory/main.R - works
  2. Rscript lint.R - works
  3. Rscript lint_box_path.R - fails with:
    Error: Linter 'box_mod_fun_exists_linter' failed in <snip>/directory/main.R: unable to load module “./foo”; not found in “.”
    Execution halted
    

Reason

The helper function for determining the "working directory of a module" uses the box.path value if it's set:

get_module_working_dir <- function(source_expression) {
box_path <- getOption("box.path")
if (is.null(box_path)) {
working_dir <- fs::path_dir(source_expression$filename)
} else {
working_dir <- box_path
}
working_dir
}

This is contradicting {box} docs I linked above.

Modules are searched in the module search path, given by getOption('box.path').
[...]
Local import declarations (that is, module prefixes that start with ./ or ../) never use the search path to find the module. Instead, only the current module’s directory (for ./) or the parent module’s directory (for ../) is looked at.


I am not sure if this is what you, @tyner, have been getting at or if it's an entirely separate issue.

@tyner
Copy link
Author

tyner commented Dec 15, 2024

Thank you @TymekDev, your analysis is spot on and I greatly appreciate it.

Looking back, my initial post was flawed/ambiguous -- I didn't make it clear what the 'test directory' was, and was unaware that box::use(./Foo[foo]) was entirely bypassing the box.path option.

Also thank you for pointing out that box.linters:::get_module_working_dir seems inconsistent in this regard. For now, leaving the thread open, in case further discussion is warranted on that.

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