Skip to content

Commit

Permalink
Code review
Browse files Browse the repository at this point in the history
  • Loading branch information
Andy Hanson committed Mar 5, 2018
1 parent a40fab8 commit 590faf0
Show file tree
Hide file tree
Showing 17 changed files with 168 additions and 156 deletions.
20 changes: 9 additions & 11 deletions src/harness/fourslash.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -365,7 +365,6 @@ namespace FourSlash {

function memoWrap(ls: ts.LanguageService, target: TestState): ts.LanguageService {
const cacheableMembers: (keyof typeof ls)[] = [
"getCompletionsAtPosition",
"getCompletionEntryDetails",
"getCompletionEntrySymbol",
"getQuickInfoAtPosition",
Expand Down Expand Up @@ -1221,7 +1220,7 @@ Actual: ${stringify(fullActual)}`);
}

private getCompletionListAtCaret(options?: FourSlashInterface.CompletionsAtOptions): ts.CompletionInfo {
return this.languageService.getCompletionsAtPosition(this.activeFile.fileName, this.currentCaretPosition, options, options && options.settings);
return this.languageService.getCompletionsAtPosition(this.activeFile.fileName, this.currentCaretPosition, options);
}

private getCompletionEntryDetails(entryName: string, source?: string): ts.CompletionEntryDetails {
Expand Down Expand Up @@ -1719,7 +1718,7 @@ Actual: ${stringify(fullActual)}`);
Harness.IO.log(stringify(sigHelp));
}

public printCompletionListMembers(options: ts.GetCompletionsAtPositionOptions | undefined) {
public printCompletionListMembers(options: ts.Options | undefined) {
const completions = this.getCompletionListAtCaret(options);
this.printMembersOrCompletions(completions);
}
Expand Down Expand Up @@ -1818,7 +1817,7 @@ Actual: ${stringify(fullActual)}`);
}
else if (prevChar === " " && /A-Za-z_/.test(ch)) {
/* Completions */
this.languageService.getCompletionsAtPosition(this.activeFile.fileName, offset, ts.defaultCompletionOptions, ts.defaultServicesSettings);
this.languageService.getCompletionsAtPosition(this.activeFile.fileName, offset, ts.defaultOptions);
}

if (i % checkCadence === 0) {
Expand Down Expand Up @@ -2393,7 +2392,7 @@ Actual: ${stringify(fullActual)}`);
public applyCodeActionFromCompletion(markerName: string, options: FourSlashInterface.VerifyCompletionActionOptions) {
this.goToMarker(markerName);

const actualCompletion = this.getCompletionListAtCaret({ ...ts.defaultCompletionOptions, includeExternalModuleExports: true }).entries.find(e =>
const actualCompletion = this.getCompletionListAtCaret({ ...ts.defaultOptions, includeExternalModuleExports: true }).entries.find(e =>
e.name === options.name && e.source === options.source);

if (!actualCompletion.hasAction) {
Expand Down Expand Up @@ -2445,7 +2444,7 @@ Actual: ${stringify(fullActual)}`);
const { fixId, newFileContent } = options;
const fixIds = ts.mapDefined(this.getCodeFixes(this.activeFile.fileName), a => a.fixId);
ts.Debug.assert(ts.contains(fixIds, fixId), "No available code fix has that group id.", () => `Expected '${fixId}'. Available action ids: ${fixIds}`);
const { changes, commands } = this.languageService.getCombinedCodeFix({ type: "file", fileName: this.activeFile.fileName }, fixId, this.formatCodeSettings);
const { changes, commands } = this.languageService.getCombinedCodeFix({ type: "file", fileName: this.activeFile.fileName }, fixId, this.formatCodeSettings, ts.defaultOptions);
assert.deepEqual(commands, options.commands);
assert(changes.every(c => c.fileName === this.activeFile.fileName), "TODO: support testing codefixes that touch multiple files");
this.applyChanges(changes);
Expand Down Expand Up @@ -2525,7 +2524,7 @@ Actual: ${stringify(fullActual)}`);
return;
}

return this.languageService.getCodeFixesAtPosition(fileName, diagnostic.start, diagnostic.start + diagnostic.length, [diagnostic.code], this.formatCodeSettings);
return this.languageService.getCodeFixesAtPosition(fileName, diagnostic.start, diagnostic.start + diagnostic.length, [diagnostic.code], this.formatCodeSettings, ts.defaultOptions);
});
}

Expand Down Expand Up @@ -4419,7 +4418,7 @@ namespace FourSlashInterface {
this.state.printCurrentSignatureHelp();
}

public printCompletionListMembers(options: ts.GetCompletionsAtPositionOptions | undefined) {
public printCompletionListMembers(options: ts.Options | undefined) {
this.state.printCompletionListMembers(options);
}

Expand Down Expand Up @@ -4616,12 +4615,11 @@ namespace FourSlashInterface {
}

export type ExpectedCompletionEntry = string | { name: string, insertText?: string, replacementSpan?: FourSlash.Range };
export interface CompletionsAtOptions extends ts.GetCompletionsAtPositionOptions {
export interface CompletionsAtOptions extends Partial<ts.Options> {
isNewIdentifierLocation?: boolean;
settings?: ts.ServicesSettings;
}

export interface VerifyCompletionListContainsOptions extends ts.GetCompletionsAtPositionOptions {
export interface VerifyCompletionListContainsOptions extends ts.Options {
sourceDisplay: string;
isRecommended?: true;
insertText?: string;
Expand Down
4 changes: 2 additions & 2 deletions src/harness/harnessLanguageService.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -417,8 +417,8 @@ namespace Harness.LanguageService {
getEncodedSemanticClassifications(fileName: string, span: ts.TextSpan): ts.Classifications {
return unwrapJSONCallResult(this.shim.getEncodedSemanticClassifications(fileName, span.start, span.length));
}
getCompletionsAtPosition(fileName: string, position: number, options: ts.GetCompletionsAtPositionOptions | undefined, settings: ts.ServicesSettings): ts.CompletionInfo {
return unwrapJSONCallResult(this.shim.getCompletionsAtPosition(fileName, position, options, settings));
getCompletionsAtPosition(fileName: string, position: number, options: ts.Options | undefined): ts.CompletionInfo {
return unwrapJSONCallResult(this.shim.getCompletionsAtPosition(fileName, position, options));
}
getCompletionEntryDetails(fileName: string, position: number, entryName: string, options: ts.FormatCodeOptions | undefined, source: string | undefined): ts.CompletionEntryDetails {
return unwrapJSONCallResult(this.shim.getCompletionEntryDetails(fileName, position, entryName, JSON.stringify(options), source));
Expand Down
16 changes: 8 additions & 8 deletions src/harness/unittests/tsserverProjectSystem.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1321,13 +1321,13 @@ namespace ts.projectSystem {
service.checkNumberOfProjects({ externalProjects: 1 });
checkProjectActualFiles(service.externalProjects[0], [f1.path, f2.path, libFile.path]);

const completions1 = service.externalProjects[0].getLanguageService().getCompletionsAtPosition(f1.path, 2, defaultCompletionOptions, defaultServicesSettings);
const completions1 = service.externalProjects[0].getLanguageService().getCompletionsAtPosition(f1.path, 2, defaultOptions);
// should contain completions for string
assert.isTrue(completions1.entries.some(e => e.name === "charAt"), "should contain 'charAt'");
assert.isFalse(completions1.entries.some(e => e.name === "toExponential"), "should not contain 'toExponential'");

service.closeClientFile(f2.path);
const completions2 = service.externalProjects[0].getLanguageService().getCompletionsAtPosition(f1.path, 2, defaultCompletionOptions, defaultServicesSettings);
const completions2 = service.externalProjects[0].getLanguageService().getCompletionsAtPosition(f1.path, 2, defaultOptions);
// should contain completions for string
assert.isFalse(completions2.entries.some(e => e.name === "charAt"), "should not contain 'charAt'");
assert.isTrue(completions2.entries.some(e => e.name === "toExponential"), "should contain 'toExponential'");
Expand All @@ -1353,11 +1353,11 @@ namespace ts.projectSystem {
service.checkNumberOfProjects({ externalProjects: 1 });
checkProjectActualFiles(service.externalProjects[0], [f1.path, f2.path, libFile.path]);

const completions1 = service.externalProjects[0].getLanguageService().getCompletionsAtPosition(f1.path, 0, defaultCompletionOptions, defaultServicesSettings);
const completions1 = service.externalProjects[0].getLanguageService().getCompletionsAtPosition(f1.path, 0, defaultOptions);
assert.isTrue(completions1.entries.some(e => e.name === "somelongname"), "should contain 'somelongname'");

service.closeClientFile(f2.path);
const completions2 = service.externalProjects[0].getLanguageService().getCompletionsAtPosition(f1.path, 0, defaultCompletionOptions, defaultServicesSettings);
const completions2 = service.externalProjects[0].getLanguageService().getCompletionsAtPosition(f1.path, 0, defaultOptions);
assert.isFalse(completions2.entries.some(e => e.name === "somelongname"), "should not contain 'somelongname'");
const sf2 = service.externalProjects[0].getLanguageService().getProgram().getSourceFile(f2.path);
assert.equal(sf2.text, "");
Expand Down Expand Up @@ -1962,7 +1962,7 @@ namespace ts.projectSystem {

// Check identifiers defined in HTML content are available in .ts file
const project = configuredProjectAt(projectService, 0);
let completions = project.getLanguageService().getCompletionsAtPosition(file1.path, 1, defaultCompletionOptions, defaultServicesSettings);
let completions = project.getLanguageService().getCompletionsAtPosition(file1.path, 1, defaultOptions);
assert(completions && completions.entries[0].name === "hello", `expected entry hello to be in completion list`);

// Close HTML file
Expand All @@ -1976,7 +1976,7 @@ namespace ts.projectSystem {
checkProjectActualFiles(configuredProjectAt(projectService, 0), [file1.path, file2.path, config.path]);

// Check identifiers defined in HTML content are not available in .ts file
completions = project.getLanguageService().getCompletionsAtPosition(file1.path, 5, defaultCompletionOptions, defaultServicesSettings);
completions = project.getLanguageService().getCompletionsAtPosition(file1.path, 5, defaultOptions);
assert(completions && completions.entries[0].name !== "hello", `unexpected hello entry in completion list`);
});

Expand Down Expand Up @@ -2629,7 +2629,7 @@ namespace ts.projectSystem {
assert.equal(lastEvent.data.project, project, "project name");
assert.isFalse(lastEvent.data.languageServiceEnabled, "Language service state");

const options = projectService.getFormatCodeOptions(f1.path as ts.server.NormalizedPath);
const options = projectService.getFormatCodeOptions(f1.path as server.NormalizedPath);
const edits = project.getLanguageService().getFormattingEditsForDocument(f1.path, options);
assert.deepEqual(edits, [{ span: createTextSpan(/*start*/ 7, /*length*/ 3), newText: " " }]);
});
Expand Down Expand Up @@ -3525,7 +3525,7 @@ namespace ts.projectSystem {
const projectService = createProjectService(host);
projectService.openClientFile(f1.path);

const defaultSettings = projectService.getFormatCodeOptions(f1.path as ts.server.NormalizedPath);
const defaultSettings = projectService.getFormatCodeOptions(f1.path as server.NormalizedPath);

// set global settings
const newGlobalSettings1 = clone(defaultSettings);
Expand Down
5 changes: 3 additions & 2 deletions src/server/client.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -169,8 +169,9 @@ namespace ts.server {
};
}

getCompletionsAtPosition(fileName: string, position: number, options: GetCompletionsAtPositionOptions | undefined): CompletionInfo {
const args: protocol.CompletionsRequestArgs = { ...this.createFileLocationRequestArgs(fileName, position), ...options };
getCompletionsAtPosition(fileName: string, position: number, _settings: Options | undefined): CompletionInfo {
// Not passing along 'settings' because server should already have those from the 'configure' command
const args: protocol.CompletionsRequestArgs = this.createFileLocationRequestArgs(fileName, position);

const request = this.processRequest<protocol.CompletionsRequest>(CommandNames.Completions, args);
const response = this.processResponse<protocol.CompletionsResponse>(request);
Expand Down
14 changes: 7 additions & 7 deletions src/server/editorServices.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -200,7 +200,7 @@ namespace ts.server {

export interface HostConfiguration {
formatCodeOptions: FormatCodeSettings;
servicesOptions: ServicesSettings;
options: Options;
hostInfo: string;
extraFileExtensions?: JsFileExtensionInfo[];
}
Expand Down Expand Up @@ -443,7 +443,7 @@ namespace ts.server {

this.hostConfiguration = {
formatCodeOptions: getDefaultFormatCodeSettings(this.host),
servicesOptions: defaultServicesSettings,
options: defaultOptions,
hostInfo: "Unknown host",
extraFileExtensions: []
};
Expand Down Expand Up @@ -710,9 +710,9 @@ namespace ts.server {
return info && info.getFormatCodeSettings() || this.hostConfiguration.formatCodeOptions;
}

getServicesSettings(file: NormalizedPath) {
getOptions(file: NormalizedPath): Options {
const info = this.getScriptInfoForNormalizedPath(file);
return info && info.getServicesSettings() || this.hostConfiguration.servicesOptions;
return info && info.getOptions() || this.hostConfiguration.options;
}

private onSourceFileChanged(fileName: NormalizedPath, eventKind: FileWatcherEventKind) {
Expand Down Expand Up @@ -1830,7 +1830,7 @@ namespace ts.server {
if (args.file) {
const info = this.getScriptInfoForNormalizedPath(toNormalizedPath(args.file));
if (info) {
info.setSettings(convertFormatOptions(args.formatOptions), args.servicesOptions);
info.setOptions(convertFormatOptions(args.formatOptions), args.options);
this.logger.info(`Host configuration update for file ${args.file}`);
}
}
Expand All @@ -1843,8 +1843,8 @@ namespace ts.server {
mergeMapLikes(this.hostConfiguration.formatCodeOptions, convertFormatOptions(args.formatOptions));
this.logger.info("Format host information updated");
}
if (args.servicesOptions) {
mergeMapLikes(this.hostConfiguration.servicesOptions, args.servicesOptions);
if (args.options) {
mergeMapLikes(this.hostConfiguration.options, args.options);
}
if (args.extraFileExtensions) {
this.hostConfiguration.extraFileExtensions = args.extraFileExtensions;
Expand Down
26 changes: 17 additions & 9 deletions src/server/protocol.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1232,7 +1232,7 @@ namespace ts.server.protocol {
*/
formatOptions?: FormatCodeSettings;

servicesOptions?: ServicesSettings;
options?: Options;

/**
* The host's additional supported .js file extensions
Expand Down Expand Up @@ -1709,15 +1709,13 @@ namespace ts.server.protocol {
*/
prefix?: string;
/**
* If enabled, TypeScript will search through all external modules' exports and add them to the completions list.
* This affects lone identifier completions but not completions on the right hand side of `obj.`.
* @deprecated Use Options
*/
includeExternalModuleExports: boolean;
includeExternalModuleExports?: boolean;
/**
* If enabled, the completion list will include completions with invalid identifier names.
* For those entries, The `insertText` and `replacementSpan` properties will be set to change from `.x` property access to `["x"]`.
* @deprecated Use Options
*/
includeInsertTextCompletions: boolean;
includeInsertTextCompletions?: boolean;
}

/**
Expand Down Expand Up @@ -2590,8 +2588,18 @@ namespace ts.server.protocol {
insertSpaceBeforeTypeAnnotation?: boolean;
}

export interface ServicesSettings {
quote: '"' | "'";
export interface Options {
quote: "double" | "single";
/**
* If enabled, TypeScript will search through all external modules' exports and add them to the completions list.
* This affects lone identifier completions but not completions on the right hand side of `obj.`.
*/
includeExternalModuleExports: boolean;
/**
* If enabled, the completion list will include completions with invalid identifier names.
* For those entries, The `insertText` and `replacementSpan` properties will be set to change from `.x` property access to `["x"]`.
*/
includeInsertTextCompletions: boolean;
}

export interface CompilerOptions {
Expand Down
14 changes: 7 additions & 7 deletions src/server/scriptInfo.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -205,7 +205,7 @@ namespace ts.server {
*/
readonly containingProjects: Project[] = [];
private formatSettings: FormatCodeSettings | undefined;
private servicesSettings: ServicesSettings | undefined;
private options: Options | undefined;

/* @internal */
fileWatcher: FileWatcher;
Expand Down Expand Up @@ -295,7 +295,7 @@ namespace ts.server {
}

getFormatCodeSettings(): FormatCodeSettings { return this.formatSettings; }
getServicesSettings(): ServicesSettings { return this.servicesSettings; }
getOptions(): Options { return this.options; }

attachToProject(project: Project): boolean {
const isNew = !this.isAttached(project);
Expand Down Expand Up @@ -388,19 +388,19 @@ namespace ts.server {
}
}

setSettings(formatSettings: FormatCodeSettings, servicesSettings: ServicesSettings): void {
setOptions(formatSettings: FormatCodeSettings, options: Options): void {
if (formatSettings) {
if (!this.formatSettings) {
this.formatSettings = getDefaultFormatCodeSettings(this.host);
}
mergeMapLikes(this.formatSettings, formatSettings);
}

if (servicesSettings) {
if (!this.servicesSettings) {
this.servicesSettings = clone(defaultServicesSettings);
if (options) {
if (!this.options) {
this.options = clone(defaultOptions);
}
mergeMapLikes(this.servicesSettings, servicesSettings);
mergeMapLikes(this.options, options);
}
}

Expand Down
Loading

0 comments on commit 590faf0

Please sign in to comment.