tealdeer-rs / tealdeer

A very fast implementation of tldr in Rust.
https://tealdeer-rs.github.io/tealdeer/
Apache License 2.0
4.17k stars 123 forks source link

Implement `cache_dir` config file option #266

Closed niklasmohrin closed 2 years ago

niklasmohrin commented 2 years ago

Closes #156, supersedes #212, unless that catches up and gets merged first :)

From the commit message:

This required that the Cache always holds a PathBuf with its base path and a cached PathBuf of the pages directory. This implies that some cache operations (such as clearing) are now instance methods instead of class methods, because the instance variables are used (we always re-resolved the paths before).

On the way, I stumbled upon some oddities and added FIXMEs. Please let me know what you think about them :)

TODO

niklasmohrin commented 2 years ago

I noticed that while developing, I have to think about preconditions / invariants of the cache directory a lot. Most recently, I noticed that in list_pages we dont error if the cache doesn't exist and in clear we check for existence again. Apart from the mental overhead, this is also unnecessary work. At first I thought we could wrap each of the invariants into a Once so that we can just sprinkle them everywhere without performance drawbacks, but this would also make every function fallible. I now think that we should use this pattern:

struct Cache<State: CacheState> {
    base_path: PathBuf,
    state: State,
}

struct Unverified {
    pages_path: Lazy<PathBuf>,
    base_metadata: Lazy<Metadata>,
    pages_metadata: Lazy<Metadata>,
}
struct Verified {
    pages_path: PathBuf,
    base_metadata: Metadata,
    pages_metadata: Metadata,
}

impl Cache<Unverified> {
    pub fn verify(self) -> Cache<Verified> { ... }
}

This way, we can lock some operations behind a verification step on the type level and make a clear distinction which steps are needed to be able to perform a certain operation on the cache. (I thought the pattern was called type-state-pattern, but I couldn't find it again; I think I remember rocket used it as a creational pattern at some point)

dbrgn commented 2 years ago

Your suggestion to use the type-state pattern sounds interesting, but is there any reason to construct a cache that isn't backed by a valid directory?

I think the invariant should be:

niklasmohrin commented 2 years ago

Closing in favor of #276