Skip to content

Commit

Permalink
Support Overriding PHPCS Integration Path (#89)
Browse files Browse the repository at this point in the history
* Added Special Options Configuration

There are currently some very specific use-cases that
we don't support. In order to add features like
that without bloating our config we can use
an aggregate config setting that only gets
filled out when a user needs to set
one of the parameters.

The first of these is an option to override the path to
the extension's PHPCS integration files.
  • Loading branch information
ObliviousHarmony authored Jan 26, 2024
1 parent 51a02b5 commit 84b783a
Show file tree
Hide file tree
Showing 12 changed files with 101 additions and 22 deletions.
5 changes: 5 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,11 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).

## [Unreleased]
### Added
- `phpCodeSniffer.specialOptions` option to allow for supporting narrow use-cases without
needing to add options that most users will not need.
- `phpCodeSniffer.specialOptions.phpcsIntegrationPathOverride` option that allows for
overriding the path to the directory containing the extension's PHPCS integration.

## [2.2.0] - 2023-11-07
### Fixed
Expand Down
13 changes: 13 additions & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,19 @@
"default": null,
"scope": "machine-overridable"
},
"phpCodeSniffer.specialOptions": {
"type": "object",
"description": "An object of special options for the extension that serve more narrow use-cases.",
"default": {},
"properties": {
"phpcsIntegrationPathOverride": {
"type": "string",
"description": "Overrides the path to the directory that contains the extension's PHPCS integration."
}
},
"additionalProperties": false,
"scope": "resource"
},
"phpCodeSniffer.ignorePatterns": {
"type": "array",
"description": "An array of regular expressions for paths that should be ignored by the extension.",
Expand Down
10 changes: 6 additions & 4 deletions src/phpcs-report/__tests__/worker-integration.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,18 +8,18 @@ import { MockCancellationToken } from '../../__mocks__/vscode';

// We need to mock the report files because Webpack is being used to bundle them.
describe('Worker/WorkerPool Integration', () => {
let phpcsIntegrationPath: string;
let phpcsPath: string;

beforeAll(() => {
// Make sure the test knows where the real assets are located.
process.env.ASSETS_PATH = resolvePath(
phpcsIntegrationPath = resolvePath(
__dirname,
'..',
'..',
'..',
'assets'
'assets',
'phpcs-integration'
);

phpcsPath = resolvePath(
__dirname,
'..',
Expand Down Expand Up @@ -53,6 +53,7 @@ describe('Worker/WorkerPool Integration', () => {
options: {
executable: phpcsPath,
standard: 'PSR12',
phpcsIntegrationPath: phpcsIntegrationPath,
},
data: null,
};
Expand Down Expand Up @@ -81,6 +82,7 @@ describe('Worker/WorkerPool Integration', () => {
options: {
executable: phpcsPath,
standard: 'PSR12',
phpcsIntegrationPath: phpcsIntegrationPath,
},
data: null,
};
Expand Down
15 changes: 11 additions & 4 deletions src/phpcs-report/__tests__/worker.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,18 +8,18 @@ import { Worker } from '../worker';

// We need to mock the report files because Webpack is being used to bundle them.
describe('Worker', () => {
let phpcsIntegrationPath: string;
let phpcsPath: string;

beforeAll(() => {
// Make sure the test knows where the real assets are located.
process.env.ASSETS_PATH = resolvePath(
phpcsIntegrationPath = resolvePath(
__dirname,
'..',
'..',
'..',
'assets'
'assets',
'phpcs-integration'
);

phpcsPath = resolvePath(
__dirname,
'..',
Expand Down Expand Up @@ -52,6 +52,7 @@ describe('Worker', () => {
options: {
executable: phpcsPath,
standard: 'PSR12',
phpcsIntegrationPath: phpcsIntegrationPath,
},
data: null,
};
Expand All @@ -74,6 +75,7 @@ describe('Worker', () => {
options: {
executable: phpcsPath,
standard: 'PSR12',
phpcsIntegrationPath: phpcsIntegrationPath,
},
data: null,
};
Expand All @@ -98,6 +100,7 @@ describe('Worker', () => {
options: {
executable: phpcsPath,
standard: 'PSR12',
phpcsIntegrationPath: phpcsIntegrationPath,
},
data: {
code: 'PSR12.Files.OpenTag.NotAlone',
Expand Down Expand Up @@ -138,6 +141,7 @@ describe('Worker', () => {
options: {
executable: phpcsPath,
standard: 'PSR12',
phpcsIntegrationPath: phpcsIntegrationPath,
},
data: {},
};
Expand All @@ -160,6 +164,7 @@ describe('Worker', () => {
options: {
executable: phpcsPath,
standard: 'PSR12',
phpcsIntegrationPath: phpcsIntegrationPath,
},
data: null,
};
Expand All @@ -186,6 +191,7 @@ describe('Worker', () => {
options: {
executable: phpcsPath,
standard: 'PSR12',
phpcsIntegrationPath: phpcsIntegrationPath,
},
data: null,
};
Expand All @@ -208,6 +214,7 @@ describe('Worker', () => {
// Since we use custom reports, adding `-s` for sources won't break anything.
executable: phpcsPath + ' -s',
standard: 'PSR12',
phpcsIntegrationPath: phpcsIntegrationPath,
},
data: null,
};
Expand Down
1 change: 1 addition & 0 deletions src/phpcs-report/request.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import { ReportType } from './response';
export interface RequestOptions {
executable: string;
standard: string | null;
phpcsIntegrationPath: string;
}

/**
Expand Down
6 changes: 1 addition & 5 deletions src/phpcs-report/worker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -162,15 +162,11 @@ export class Worker {
resolve: (response: Response<T>) => void,
reject: (e?: unknown) => void
): ChildProcess {
// Figure out the path to the PHPCS integration.
const assetPath =
process.env.ASSETS_PATH || resolvePath(__dirname, 'assets');

// Prepare the arguments for our PHPCS process.
const processArguments = [
'-q', // Make sure custom configs never break our output.
'--report=' +
resolvePath(assetPath, 'phpcs-integration', 'VSCode.php'),
resolvePath(request.options.phpcsIntegrationPath, 'VSCode.php'),
// We want to reserve error exit codes for actual errors in the PHPCS execution since errors/warnings are expected.
'--runtime-set',
'ignore_warnings_on_exit',
Expand Down
4 changes: 4 additions & 0 deletions src/services/__tests__/configuration.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import { TextEncoder } from 'util';
import {
Configuration,
LintAction,
SpecialOptions,
SpecialStandardOptions,
} from '../configuration';
import { WorkspaceLocator } from '../workspace-locator';
Expand All @@ -29,6 +30,7 @@ type ConfigurationType = {
lintAction: LintAction;
standard: SpecialStandardOptions | string;
standardCustom: string;
specialOptions: SpecialOptions;

// Deprecated Options
executable: string | null;
Expand Down Expand Up @@ -59,6 +61,8 @@ const getDefaultConfiguration = (overrides?: Partial<ConfigurationType>) => {
return SpecialStandardOptions.Disabled;
case 'standardCustom':
return '';
case 'specialOptions':
return {};

// Deprecated Settings
case 'executable':
Expand Down
14 changes: 10 additions & 4 deletions src/services/__tests__/diagnostic-updater.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ jest.mock('../../types', () => {
});

describe('DiagnosticUpdater', () => {
let phpcsIntegrationPath: string;
let mockLogger: Logger;
let mockWorkspaceLocator: WorkspaceLocator;
let mockConfiguration: Configuration;
Expand All @@ -54,15 +55,15 @@ describe('DiagnosticUpdater', () => {
let diagnosticUpdater: DiagnosticUpdater;

beforeEach(() => {
// Make sure the test knows where the real assets are located.
process.env.ASSETS_PATH = resolvePath(
phpcsIntegrationPath = resolvePath(
__dirname,
'..',
'..',
'..',
'assets'
'..',
'assets',
'phpcs-integration'
);

mockLogger = new Logger(window);
mockWorkspaceLocator = new WorkspaceLocator(workspace);
mockConfiguration = new Configuration(workspace, mockWorkspaceLocator);
Expand Down Expand Up @@ -104,6 +105,7 @@ describe('DiagnosticUpdater', () => {
exclude: [],
lintAction: LintAction.Change,
standard: 'PSR12',
phpcsIntegrationPath: phpcsIntegrationPath,
});
jest.mocked(mockWorker).execute.mockImplementation((request) => {
expect(request).toMatchObject({
Expand All @@ -112,6 +114,7 @@ describe('DiagnosticUpdater', () => {
options: {
executable: 'phpcs-test',
standard: 'PSR12',
phpcsIntegrationPath: phpcsIntegrationPath,
},
});

Expand Down Expand Up @@ -166,6 +169,7 @@ describe('DiagnosticUpdater', () => {
exclude: [],
lintAction: LintAction.Change,
standard: 'PSR12',
phpcsIntegrationPath: phpcsIntegrationPath,
});
jest.mocked(mockWorker).execute.mockImplementation((request) => {
expect(request).toMatchObject({
Expand All @@ -174,6 +178,7 @@ describe('DiagnosticUpdater', () => {
options: {
executable: 'phpcs-test',
standard: 'PSR12',
phpcsIntegrationPath: phpcsIntegrationPath,
},
});

Expand All @@ -198,6 +203,7 @@ describe('DiagnosticUpdater', () => {
exclude: [],
lintAction: LintAction.Save,
standard: 'PSR12',
phpcsIntegrationPath: phpcsIntegrationPath,
});

return diagnosticUpdater.update(document, LintAction.Change);
Expand Down
1 change: 1 addition & 0 deletions src/services/code-action-edit-resolver.ts
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@ export class CodeActionEditResolver extends WorkerService {
options: {
executable: config.executable,
standard: config.standard,
phpcsIntegrationPath: config.phpcsIntegrationPath,
},
data: {
code: diagnostic.code as string,
Expand Down
41 changes: 41 additions & 0 deletions src/services/configuration.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { resolve as resolvePath } from 'path';
import { TextDecoder } from 'util';
import { Minimatch } from 'minimatch';
import {
Expand Down Expand Up @@ -47,6 +48,16 @@ export enum LintAction {
Force = 'Force',
}

/**
* An interface describing the narrow use-case options in the `phpCodeSniffer.specialOptions` configuration.
*/
export interface SpecialOptions {
/**
* An override for the path to the directory containing the extension's PHPCS integration files.
*/
phpcsIntegrationPathOverride?: string;
}

/**
* An interface describing the configuration parameters we can read from the filesystem.
*/
Expand All @@ -63,6 +74,7 @@ interface ParamsFromConfiguration {
exclude: RegExp[];
lintAction: LintAction;
standard: string | null;
phpcsIntegrationPath: string;
}

/**
Expand All @@ -88,6 +100,11 @@ export interface DocumentConfiguration {
* The standard we should use when executing reports.
*/
standard: string | null;

/**
* The path to the PHPCS integration files.
*/
phpcsIntegrationPath: string;
}

/**
Expand Down Expand Up @@ -210,6 +227,7 @@ export class Configuration {
exclude: fromConfig.exclude,
lintAction: fromConfig.lintAction,
standard: fromConfig.standard,
phpcsIntegrationPath: fromConfig.phpcsIntegrationPath,
};
this.cache.set(document.uri, config);

Expand Down Expand Up @@ -343,12 +361,35 @@ export class Configuration {
cancellationToken
);

const specialOptions = config.get<SpecialOptions>('specialOptions');
if (specialOptions === undefined) {
throw new ConfigurationError(
'specialOptions',
'Value must be an object.'
);
}

// Use the default integration path unless overridden.
let phpcsIntegrationPath: string;
if (specialOptions.phpcsIntegrationPathOverride) {
phpcsIntegrationPath = specialOptions.phpcsIntegrationPathOverride;
} else {
// Keep in mind that after bundling the integration files will be in a different location
// than they are in development and we need to resolve the correct path.
phpcsIntegrationPath = resolvePath(
__dirname,
'assets',
'phpcs-integration'
);
}

return {
autoExecutable,
executable,
exclude,
lintAction,
standard,
phpcsIntegrationPath,
};
}

Expand Down
12 changes: 7 additions & 5 deletions src/services/diagnostic-updater.ts
Original file line number Diff line number Diff line change
Expand Up @@ -110,20 +110,20 @@ export class DiagnosticUpdater extends WorkerService {

this.configuration
.get(document, cancellationToken)
.then((configuration) => {
.then((config) => {
// Allow users to decide when the diagnostics are updated.
switch (lintAction) {
case LintAction.Change:
// Allow users to restrict updates to explicit save actions.
if (configuration.lintAction === LintAction.Save) {
if (config.lintAction === LintAction.Save) {
throw new UpdatePreventedError();
}

break;

case LintAction.Save:
// When linting on change, we will always have the latest diagnostics, and don't need to update.
if (configuration.lintAction === LintAction.Change) {
if (config.lintAction === LintAction.Change) {
throw new UpdatePreventedError();
}
break;
Expand All @@ -147,8 +147,10 @@ export class DiagnosticUpdater extends WorkerService {
documentPath: document.uri.fsPath,
documentContent: document.getText(),
options: {
executable: configuration.executable,
standard: configuration.standard,
executable: config.executable,
standard: config.standard,
phpcsIntegrationPath:
config.phpcsIntegrationPath,
},
data: null,
};
Expand Down
Loading

0 comments on commit 84b783a

Please sign in to comment.