r-lib / lintr

Static Code Analysis for R
https://lintr.r-lib.org
Other
1.19k stars 184 forks source link

Is caching not working as expected? #2642

Open IndrajeetPatil opened 2 months ago

IndrajeetPatil commented 2 months ago

I was expecting that the second call here would be faster because it would use cache, but that's not what I see (at least locally).

library(lintr)

local_dir <- "/Users/indrajeetpatil/Documents/GitHub/lintr/R"

# the same behaviour is also seen with `lint_package()`
lint_dir(local_dir, cache = TRUE)
lint_dir(local_dir)                # I'd have expected this to be quick 

Either I am doing something wrong, or the caching is not working as expected.

IndrajeetPatil commented 2 months ago
> system.time(lintr::lint_dir("~/Documents/GitHub/lintr/R", cache = TRUE))
  |====================================================================================================================| 100%
   user  system elapsed 
 49.759   1.422  52.014 
> system.time(lintr::lint_dir("~/Documents/GitHub/lintr/R"))
  |====================================================================================================================| 100%
   user  system elapsed 
 58.436   1.401  61.078 
AshesITR commented 1 month ago

Does it work when specifying cache = TRUE both times?

MichaelChirico commented 1 month ago

This is WAI IMO, cache=FALSE should never try and read an existing cache (which may have been invalidated):

https://github.com/r-lib/lintr/blob/f2da882ce6d8889842b8b46294b367ff734f6513/R/cache.R#L28

Possibly, we could consider allowing cache=NULL to mean "use cache if found at default path", and further consider making that the default, but the current behavior for FALSE is correct.

IndrajeetPatil commented 1 month ago

Okay, I clearly have an incorrect mental model of {lintr}’s caching algorithm. 

As a user, here is how I thought it worked:

Admittedly, this UX expectation comes from how caching works in {styler}, and I find it to be much more intuitive than what’s going on in {lintr}.

But I am happy to be challenged on that point :) 

AshesITR commented 1 month ago

I'm happy with revisiting the cache UX.

MichaelChirico commented 1 month ago

Noting: if cache = 'my_dir', I don't know that we have any way to recover that in the nest invocation. You'll have to keep calling with cache='my_dir'.

Anyone, +1 to revisiting. We don't have a cache_clear() function now, is important context for my reasoning. So if we add one, that changes things.