Skip to content

Commit

Permalink
fix: Adjust the way glob roots are calculated
Browse files Browse the repository at this point in the history 
Try to support the "assumed" behavior.
  • Loading branch information
@Jason3S
Jason3S committed Mar 1, 2021
1 parent af1fed7 commit d1a7fa7
Show file tree
Hide file tree
Showing 3 changed files with 27 additions and 7 deletions.
10 changes: 5 additions & 5 deletions packages/cspell-lib/src/Settings/CSpellSettingsServer.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -318,9 +318,9 @@ describe('Validate Glob resolution', () => {

test('normalized settings', () => {
expect(sampleSettings).not.toEqual(sampleSettingsV1);
expect(sampleSettings.globRoot).not.toEqual(sampleSettingsV1.globRoot);
expect(sampleSettings.globRoot).toEqual(sampleSettingsV1.globRoot);
expect(sampleSettings.globRoot).toBe(__dirname);
expect(sampleSettingsV1.globRoot).toBe(process.cwd());
expect(sampleSettingsV1.globRoot).toContain(process.cwd());
expect(sampleSettings.ignorePaths).toEqual(
expect.arrayContaining([
{ glob: 'node_modules', root: sampleSettings.globRoot, source: sampleSettingsFilename },
Expand All @@ -339,14 +339,14 @@ describe('Validate Glob resolution', () => {
const settingsV1 = normalizeSettings(rawSampleSettingsV1, __filename);

expect(settingsV).toEqual(sampleSettings);
expect(settingsV1).not.toEqual(sampleSettingsV1);
expect(settingsV1).toEqual(sampleSettingsV1);

delete settingsV1.version;
expect(settingsV1).toEqual(sampleSettings);
});

test('Using ENV_CSPELL_GLOB_ROOT as __dirname/..', () => {
process.env[ENV_CSPELL_GLOB_ROOT] = path.resolve(__dirname, '..');
test('Using ENV_CSPELL_GLOB_ROOT as without shared hierarchy', () => {
process.env[ENV_CSPELL_GLOB_ROOT] = path.resolve(__dirname, '../../samples');
const settingsV = normalizeSettings(rawSampleSettings, __filename);
const settingsV1 = normalizeSettings(rawSampleSettingsV1, __filename);

Expand Down
19 changes: 17 additions & 2 deletions packages/cspell-lib/src/Settings/CSpellSettingsServer.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -357,11 +357,13 @@ function merge(left: CSpellSettings, right: CSpellSettings): CSpellSettings {
const includeRegExpList = takeRightOtherwiseLeft(left.includeRegExpList, right.includeRegExpList);

const optionals = includeRegExpList?.length ? { includeRegExpList } : {};
const version = max(left.version, right.version);

const settings: CSpellSettings = {
...left,
...right,
...optionals,
version,
id: [leftId, rightId].join('|'),
name: [left.name || '', right.name || ''].join('|'),
words: mergeList(left.words, right.words),
Expand All @@ -380,8 +382,8 @@ function merge(left: CSpellSettings, right: CSpellSettings): CSpellSettings {
),
enabled: right.enabled !== undefined ? right.enabled : left.enabled,
files: mergeListUnique(left.files, right.files),
ignorePaths: versionBasedMergeList(left.ignorePaths, right.ignorePaths, right.version),
overrides: versionBasedMergeList(left.overrides, right.overrides, right.version),
ignorePaths: versionBasedMergeList(left.ignorePaths, right.ignorePaths, version),
overrides: versionBasedMergeList(left.overrides, right.overrides, version),
source: mergeSources(left, right),
globRoot: undefined,
import: undefined,
Expand Down Expand Up @@ -558,6 +560,16 @@ function mergeSources(left: CSpellSettings, right: CSpellSettings): Source {
};
}

function max<T>(a: undefined, b: undefined): undefined;
function max<T>(a: T, b: undefined): T;
function max<T>(a: undefined, b: T): T;
function max<T>(a: T | undefined, b: T | undefined): T | undefined;
function max<T>(a: T | undefined, b: T | undefined): T | undefined {
if (a === undefined) return b;
if (b === undefined) return a;
return a > b ? a : b;
}

/**
* Return a list of Setting Sources used to create this Setting.
* @param settings the settings to search
Expand Down Expand Up @@ -620,6 +632,9 @@ function resolveGlobRoot(settings: CSpellSettings, pathToSettingsFile: string):
settings.globRoot ??
(settings.version === '0.1' || (envGlobRoot && !settings.version) ? defaultGlobRoot : settingsFileDir);
const globRoot = path.resolve(settingsFileDir, rawRoot.replace('${cwd}', cwd));
if (settings.globRoot) return globRoot;
if (settingsFileDir.startsWith(globRoot)) return settingsFileDir;
if (globRoot.startsWith(settingsFileDir)) return settingsFileDir;
return globRoot;
}

Expand Down
5 changes: 5 additions & 0 deletions packages/cspell-lib/src/SpellingDictionary/DictionaryLoader.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,11 @@ function calcKey(uri: string) {
return [uri, loaderType].join('|');
}

/**
* Check to see if any of the cached dictionaries have changed. If one has changed, reload it.
* @param maxAge - Only check the dictionary if it has been at least `maxAge` ms since the last check.
* @param now - optional timestamp representing now. (Mostly used in testing)
*/
export async function refreshCacheEntries(maxAge = MAX_AGE, now = Date.now()): Promise<void> {
await Promise.all([...dictionaryCache].map(([, entry]) => refreshEntry(entry, maxAge, now)));
}
Expand Down

5 comments on commit d1a7fa7

@timwsuqld
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Jason3S Can you explain this change a bit more? I suddenly have a build failing with lots of my ignorePaths no longer working. E.g. I have "html/**" as one of my ignorePaths and suddenly files under html/ are being scanned when before they would be ignored

@timwsuqld
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Jason3S Seems like it was #913 with the change to the globRoot. Can you please add a "Breaking Change" section to the changelog the gives details of this. Thanks

@Jason3S
Copy link
Collaborator Author

@Jason3S Jason3S commented on d1a7fa7 Mar 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@timwsuqld Sorry about breaking your build.

I would like to understand how this change impacted you. Do you happen to have a sample repo I can look at?

Specifically where is the configuration file (cspell.json) file located relative to the files being checked.

@timwsuqld
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Jason3S unfortunately no sample repo that's public. But simply we had a config located at tests/spellcheck-settings.json similar to this:

{
    "version": "0.1",
    "language": "en-GB",
    "flagWords": [
        "hte"
    ],
    "globRoot": "../",
    "ignorePaths": [
        "**/*.svg",
        "**/*.xls",
        "**/*.xlsx",
	"**/*.pdf",

We call cspell in CI with the command cspell --config=tests/spellcheck-settings.json $(git ls-files) (after install npm install -g cspell, which I later realised was my bad for not locking it to a specific version)

The simple fix was adding a globRoot

{
    "version": "0.1",
    "language": "en-GB",
    "flagWords": [
        "hte"
    ],
    "globRoot": "../",
    "ignorePaths": [
        "**/*.svg",
        "**/*.xls",
        "**/*.xlsx",
	"**/*.pdf",

It was my bad for not locking the cspell version. Having a BREAKING changes section to a changelog would also help, as it would let me quickly search for what breaking changes the newer release had, and ideally would point me to documentation explaining what I needed to change to have things working correctly.

@Jason3S
Copy link
Collaborator Author

@Jason3S Jason3S commented on d1a7fa7 Mar 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, a BREAKING section would help. The comments were in the pull requests, but did not make it into the CHANGELOG.

Please sign in to comment.