Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix issues with the .gitignore loading for pathFilterer #1325

Merged
merged 2 commits into from
Oct 11, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
128 changes: 128 additions & 0 deletions src/LanguageServer.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import type { Project } from './lsp/Project';
import { LogLevel, Logger, createLogger } from './logging';
import { DiagnosticMessages } from './DiagnosticMessages';
import { standardizePath } from 'roku-deploy';
import undent from 'undent';

const sinon = createSandbox();

Expand Down Expand Up @@ -578,6 +579,133 @@ describe('LanguageServer', () => {
});
});

describe('rebuildPathFilterer', () => {
let workspaceConfigs: WorkspaceConfigWithExtras[] = [];
beforeEach(() => {
workspaceConfigs = [
{
bsconfigPath: undefined,
languageServer: {
enableThreading: true,
logLevel: 'info'
},
workspaceFolder: workspacePath,
excludePatterns: []
} as WorkspaceConfigWithExtras
];
server['connection'] = connection as any;
sinon.stub(server as any, 'getWorkspaceConfigs').callsFake(() => Promise.resolve(workspaceConfigs));
});

it('allows files from dist by default', async () => {
const filterer = await server['rebuildPathFilterer']();
//certain files are allowed through by default
expect(
filterer.filter([
s`${rootDir}/manifest`,
s`${rootDir}/dist/file.brs`,
s`${rootDir}/source/file.brs`
])
).to.eql([
s`${rootDir}/manifest`,
s`${rootDir}/dist/file.brs`,
s`${rootDir}/source/file.brs`
]);
});

it('filters out some standard locations by default', async () => {
const filterer = await server['rebuildPathFilterer']();

expect(
filterer.filter([
s`${workspacePath}/node_modules/file.brs`,
s`${workspacePath}/.git/file.brs`,
s`${workspacePath}/out/file.brs`,
s`${workspacePath}/.roku-deploy-staging/file.brs`
])
).to.eql([]);
});

it('properly handles a .gitignore list', async () => {
fsExtra.outputFileSync(s`${workspacePath}/.gitignore`, undent`
dist/
`);

const filterer = await server['rebuildPathFilterer']();

//filters files that appear in a .gitignore list
expect(
filterer.filter([
s`${workspacePath}/src/source/file.brs`,
s`${workspacePath}/dist/source/file.brs`
])
).to.eql([
s`${workspacePath}/src/source/file.brs`
]);
});

it('does not crash for path outside of workspaceFolder', async () => {
fsExtra.outputFileSync(s`${workspacePath}/.gitignore`, undent`
dist/
`);

const filterer = await server['rebuildPathFilterer']();

//filters files that appear in a .gitignore list
expect(
filterer.filter([
s`${workspacePath}/../flavor1/src/source/file.brs`
])
).to.eql([
//since the path is outside the workspace, it does not match the .gitignore patter, and thus is not excluded
s`${workspacePath}/../flavor1/src/source/file.brs`
]);
});

it('a gitignore file from any workspace will apply to all workspaces', async () => {
workspaceConfigs = [{
bsconfigPath: undefined,
languageServer: {
enableThreading: true,
logLevel: 'info'
},
workspaceFolder: s`${tempDir}/flavor1`,
excludePatterns: []
}, {
bsconfigPath: undefined,
languageServer: {
enableThreading: true,
logLevel: 'info'
},
workspaceFolder: s`${tempDir}/flavor2`,
excludePatterns: []
}] as WorkspaceConfigWithExtras[];
fsExtra.outputFileSync(s`${workspaceConfigs[0].workspaceFolder}/.gitignore`, undent`
dist/
`);
fsExtra.outputFileSync(s`${workspaceConfigs[1].workspaceFolder}/.gitignore`, undent`
out/
`);

const filterer = await server['rebuildPathFilterer']();

//filters files that appear in a .gitignore list
expect(
filterer.filter([
//included files
s`${workspaceConfigs[0].workspaceFolder}/src/source/file.brs`,
s`${workspaceConfigs[1].workspaceFolder}/src/source/file.brs`,
//excluded files
s`${workspaceConfigs[0].workspaceFolder}/dist/source/file.brs`,
s`${workspaceConfigs[1].workspaceFolder}/out/source/file.brs`
])
).to.eql([
s`${workspaceConfigs[0].workspaceFolder}/src/source/file.brs`,
s`${workspaceConfigs[1].workspaceFolder}/src/source/file.brs`
]);
});
});

describe('onDidChangeWatchedFiles', () => {
it('does not trigger revalidates when changes are in files which are not tracked', async () => {
server.run();
Expand Down
21 changes: 13 additions & 8 deletions src/LanguageServer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -594,9 +594,9 @@ export class LanguageServer {
*/
private async rebuildPathFilterer() {
this.pathFilterer.clear();
const workspaceFolders = await this.connection.workspace.getWorkspaceFolders();
await Promise.all(workspaceFolders.map(async (workspaceFolder) => {
const rootDir = util.uriToPath(workspaceFolder.uri);
const workspaceConfigs = await this.getWorkspaceConfigs();
await Promise.all(workspaceConfigs.map(async (workspaceConfig) => {
const rootDir = util.uriToPath(workspaceConfig.workspaceFolder);

//always exclude everything from these common folders
this.pathFilterer.registerExcludeList(rootDir, [
Expand All @@ -606,17 +606,22 @@ export class LanguageServer {
'**/.roku-deploy-staging/**/*'
]);
//get any `files.exclude` patterns from the client from this workspace
const workspaceExcludeGlobs = await this.getWorkspaceExcludeGlobs(rootDir);
this.pathFilterer.registerExcludeList(rootDir, workspaceExcludeGlobs);
this.pathFilterer.registerExcludeList(rootDir, workspaceConfig.excludePatterns);

//get any .gitignore patterns from the client from this workspace
const gitignorePath = path.resolve(rootDir, '.gitignore');
if (await fsExtra.pathExists(gitignorePath)) {
const matcher = ignore().add(
const matcher = ignore({ ignoreCase: true }).add(
fsExtra.readFileSync(gitignorePath).toString()
);
this.pathFilterer.registerExcludeMatcher((path: string) => {
return matcher.test(path).ignored;
this.pathFilterer.registerExcludeMatcher((p: string) => {
const relPath = path.relative(rootDir, p);
if (ignore.isPathValid(relPath)) {
return matcher.test(relPath).ignored;
} else {
//we do not have a valid relative path, so we cannot determine if it is ignored...thus it is NOT ignored
return false;
}
});
}
}));
Expand Down
22 changes: 22 additions & 0 deletions src/lsp/PathFilterer.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,18 @@ import { PathCollection, PathFilterer } from './PathFilterer';
import { cwd, rootDir } from '../testHelpers.spec';
import { expect } from 'chai';
import { standardizePath as s } from '../util';
import { createSandbox } from 'sinon';
const sinon = createSandbox();

describe('PathFilterer', () => {
let filterer: PathFilterer;

beforeEach(() => {
filterer = new PathFilterer();
sinon.restore();
});
afterEach(() => {
sinon.restore();
});

it('allows all files through when no filters exist', () => {
Expand Down Expand Up @@ -155,4 +161,20 @@ describe('PathFilterer', () => {
).to.eql([]);
});

describe('registerExcludeMatcher', () => {
it('calls the callback function on every path', () => {
const spy = sinon.spy();
filterer.registerExcludeMatcher(spy);
filterer.filter([
s`${rootDir}/a.brs`,
s`${rootDir}/b.txt`,
s`${rootDir}/c.brs`
]);
expect(spy.getCalls().map(x => s`${x.args[0]}`)).to.eql([
s`${rootDir}/a.brs`,
s`${rootDir}/b.txt`,
s`${rootDir}/c.brs`
]);
});
});
});
1 change: 1 addition & 0 deletions src/lsp/PathFilterer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,7 @@ export class PathFilterer {
const collection = new PathCollection({
matcher: matcher
});
this.excludeCollections.push(collection);
return () => {
this.removeCollection(collection);
};
Expand Down