`subdir` concept seems flawed

[salim-b]

The documentation of root_criterion() says about param subdir:

Subdirectories to start the search in, if found

Note: Subdirectories, plural.

But the code only considers the first existing subdirectory.

Excerpt from find_root():

https://github.com/r-lib/rprojroot/blob/1019ad3b34aa302e39b0d24774e0da315f4b8edd/R/root.R#L92

and from get_start_path():

https://github.com/r-lib/rprojroot/blob/1019ad3b34aa302e39b0d24774e0da315f4b8edd/R/root.R#L122-L124

Or did I misinterpret something?

[krlmlr]

Thanks. Perhaps this could be refactored by handling all of the subdir argument in the function created by has_basename() (because that's the only place where it is used). The subdir argument to root_criterion() could then be deprecated.

[krlmlr]

Let's discuss this independently of #83 if possible.

[salim-b]

See #100.

The whole subdir concept is a bit confusing since in most cases like the has_file() criterion helper it isn't needed since we can just specify subdirs directly in filepath:

https://github.com/r-lib/rprojroot/blob/94231b4f5af82e345a812bd74abefe60a411ac76/R/root.R#L324

It happened to work before with has_basename(), the only built-in criterion helper that makes use of subdir, but in more complex cases it failed. But I admittedly couldn't come up with a really sensible example, so the added test is a bit artificial.

[krlmlr]

Thanks for the test case. For now, I've updated the documentation, also experimented with restricting the length of the argument.

The is_testthat criterion is special because we sometimes start the search in tests/testthat . I initially suggested handling all of the subdir argument inside has_basename(), this doesn't work without changing the way the main search loop works. On the other hand, I'm still hesitant to make an extensive change to the main search loop to accommodate this use case.

This leads to the main question that I forgot to ask in the beginning: what is the use case for starting the search in multiple subdirectories in parallel?

[salim-b]

what is the use case for starting the search in multiple subdirectories in parallel?

I currently don't have an actual use case, thus the arguably artificial test in #100. The updated documentation in #103 should help avoid future misinterpretation of the subdir argument, thanks.

On the other hand, I'm still hesitant to make an extensive change to the main search loop to accommodate this use case.

Ok, I understand. Although I can't really see any downsides of the updated search loop in #100, it shouldn't degrade performance (though I haven't actually benchmarked). But since I personally don't really need it, I'm kinda neutral here.

[krlmlr]

The only downside is that it's more complicated, one level of nesting deeper. I'd rather not go there without a good reason.