feat: add --cache option to lint only changed files

[mad-gooze]

Hi!

This PR adds three new cli options:

• --cache that enables caching lint results on disk
• --cache-location <path> that sets cache file location
• --cache-strategy <metadata|content> that sets cache strategy used by file-entry-cache

Cache implementation is heavily inspired by ESLint https://github.com/eslint/eslint/blob/c981fb1994cd04914042ced1980aa86b68ba7be9/lib/cli-engine/lint-result-cache.js

[Jason3S]

Wow, you have been busy! I was hoping to add a cache.
It is going to take me a bit to look through this. I need to try it out!

I was thinking it would be a good idea to keep all cspell related files in the same directory: .cspell

- .cspell/
    |-- .cspellcache/
    |-- custom-dictionary.txt
    |-- known-issues.yaml
    |-- future files.

[Jason3S]

Seems to be breaking on the Yarn pnp.js files.

./bin.js lint --cache --cache-strategy content --no-progress "**"

Hmm. I don't think it is Yarn related:

./bin.js lint --cache --cache-strategy content --no-progress --exclude "**/yarn2/**" "**":

[Jason3S]

In package.json there is a test-bin script. I suggest adding two tests to that, one for each strategy.

    "test-bin": "... 
    "test-bin-spell-cache-content": "node ./bin.js -c cspellrc.json --cache --cache-strategy content --cache-location ...",
    "test-bin-spell-cache-meta": "node ./bin.js -c cspellrc.json --cache --cache-strategy metadata --cache-location ...",

[Jason3S]

Thank you for adding this.

I think it will help a lot.

I think it is worth it to get this in with its current logic.
Note: the cache logic is not correct for a mono-repo style project (like this one) with nested cspell.json files. In a future PR, it might be worth it to handle the case when a nested config file changes.

[mad-gooze]

Note: the cache logic is not correct for a mono-repo style project (like this one) with nested cspell.json files. In a future PR, it might be worth it to handle the case when a nested config file changes.

Oh, really, i did not get the idea that cspell supports nested configs.
I think I can handle it easily here by hashing ConfigInfo for every file (in a way it's done in eslint)

[Jason3S]

I'm looking forward to getting this in.

I you would like, we can do it in two steps.

1. Fix the current PR so everything passes and I will merge it.
2. Follow up on removing .doc and the per file level config in subsequent PRs.

[mad-gooze]

I've removed doc and added per-file config hashing.

Could you review this again please?

[Jason3S]

Thank you for getting this done!

[Jason3S]

FYI: It is also possible to use the spy

// jest.mock('../../fileHelper', () => ({ readFileInfo: jest.fn() }));
mockReadFileInfo.mockImplementation(jest.fn());

[Jason3S]

Rule of thumb:

1. Mock Module.
2. Create Spy.

jest.mock('file-entry-cache');
const mockCreateFileEntryCache = jest.spyOn(FileEntryCacheModule, 'create');
mockCreateFileEntryCache.mockImplementation(
    jest.fn().mockReturnValue({
        getFileDescriptor: jest.fn(),
        reconcile: jest.fn(),
    })
);

[Jason3S]

Also worth reading:
mocked - Test helpers | ts-jest

[Jason3S]

fileEntryCache = mockCreateFileEntryCache.mock.results[0].value;

Accessing the mock like this is fragile because it depends upon Jest's implementation.
By flipping the spy logic and injecting a known cache entry, it will make things more reliable.

    const fileEntryCache = {
        getFileDescriptor: jest.fn(),
        reconcile: jest.fn(),
    };

    beforeEach(() => {
        diskCache = new DiskCache('.foobar', false);
        mockCreateFileEntryCache.mockImplementation(jest.fn().mockReturnValue(fileEntryCache));
    });

[Jason3S]

Watch out for the cast. It removed the undefined from the type.
meta can be undefined, but it is not getting checked.

        const meta = fileDescriptor.meta as CSpellCacheMeta | undefined;

or

type CSpellCacheMeta =
    | (FileDescriptor['meta'] & {
          result: CachedFileResult;
          configHash: string;
      })
    | undefined;

[Jason3S]

same as above. meta can be undefined.

[Jason3S]

Reading the package.json is also an option to ensure the version is correct.

// eslint-disable-next-line @typescript-eslint/no-var-requires
const packageVersion = require('../../../package.json').version;

// ...

expect(mockHash.mock.calls[0][0]).toContain(packageVersion);

[Jason3S]

@mad-gooze,

Thank you very much! The suggestions I made can come in later PRs.