Support services settings #22236

ghost · 2018-02-28T19:14:35Z

Start of #20619
Fixes #22342
Fixes #22249
Fixes #18169

This expands the "configure" request to include a second set of settings. It also adds the settings as a new parameter to getCompletionsAtPosition; we'll probably want to add it to many other LanguageService methods eventually.

amcasey · 2018-03-01T01:19:23Z

FYI @RyanCavanaugh

sheetalkamat · 2018-03-01T19:40:25Z

src/services/types.ts

@@ -242,7 +250,7 @@ namespace ts {
        getEncodedSyntacticClassifications(fileName: string, span: TextSpan): Classifications;
        getEncodedSemanticClassifications(fileName: string, span: TextSpan): Classifications;

-        getCompletionsAtPosition(fileName: string, position: number, options: GetCompletionsAtPositionOptions | undefined): CompletionInfo;
+        getCompletionsAtPosition(fileName: string, position: number, options: GetCompletionsAtPositionOptions | undefined, settings: ServicesSettings | undefined): CompletionInfo;


make it optional instead so that the api doesn't break

Like with the options: GetCompletionsAtPositionOptions | undefined,, this is a types breaking change but will handle it gracefully at runtime -- we want to be reminding people to send us the settings though.

mhegazy · 2018-03-01T19:57:03Z

src/server/protocol.ts

@@ -1232,6 +1232,8 @@ namespace ts.server.protocol {
         */
        formatOptions?: FormatCodeSettings;

+        servicesOptions?: ServicesSettings;


consider renaming this to just options. since we intend to make this the bag of all options that are not formatting,

Should the type be Options too?

mhegazy · 2018-03-01T19:58:43Z

src/server/session.ts

@@ -2151,6 +2150,14 @@ namespace ts.server {
                    "Error processing request. " + (<StackTraceError>err).message + "\n" + (<StackTraceError>err).stack);
            }
        }
+
+        private formatSettings(file: NormalizedPath): FormatCodeSettings {


getFormatOptions?

mhegazy · 2018-03-01T20:05:07Z

src/server/protocol.ts

@@ -2588,6 +2590,10 @@ namespace ts.server.protocol {
        insertSpaceBeforeTypeAnnotation?: boolean;
    }

+    export interface ServicesSettings {
+        quote: '"' | "'";


I would say create an enum for this, or even a literal union type that is not the quote. e.g. "double" | "single". VSCode at the end of the day will put this setting in a json file and having ppl wiring "\"" is not that great.

mhegazy · 2018-03-01T20:07:30Z

We also need this in the importfixes file, do you plan on doing this in a separate PR or in this PR?

ghost · 2018-03-01T22:27:15Z

Most services may eventually need these options -- should we just add it to all methods now instead of doing it one-by-one as we need it?

mhegazy · 2018-03-01T22:38:48Z

Most services may eventually need these options -- should we just add it to all methods now instead of doing it one-by-one as we need it?

GotoDef, findAllRefs, rename, sighelp, etc.. will not need it.. we just need it in things that will generate code, namelly completions, quickfixes, and refactoring. Formatting has its own thing.

amcasey · 2018-03-02T01:25:00Z

I'm not sure I understand the high-level goal of this change. It seems like we wanted to add an option to completion but wanted to avoid having to pass it on every call to completion (vs doing it once at the beginning)?

amcasey · 2018-03-02T01:26:13Z

src/server/editorServices.ts

@@ -200,6 +200,7 @@ namespace ts.server {

    export interface HostConfiguration {
        formatCodeOptions: FormatCodeSettings;
+        servicesOptions: ServicesSettings;


Do we have a pattern we follow when choosing between "options" and settings"?

amcasey · 2018-03-02T01:30:43Z

src/services/types.ts

@@ -242,7 +250,7 @@ namespace ts {
        getEncodedSyntacticClassifications(fileName: string, span: TextSpan): Classifications;
        getEncodedSemanticClassifications(fileName: string, span: TextSpan): Classifications;

-        getCompletionsAtPosition(fileName: string, position: number, options: GetCompletionsAtPositionOptions | undefined): CompletionInfo;
+        getCompletionsAtPosition(fileName: string, position: number, options: GetCompletionsAtPositionOptions | undefined, settings: ServicesSettings | undefined): CompletionInfo;


Are callers likely to want to pass different values for GetCompletionsAtPositionOptions each time they call getCompletionsAtPosition? Is there some way we can incorporate those options into ServicesSettings?

mhegazy · 2018-03-06T00:01:35Z

I'm not sure I understand the high-level goal of this change. It seems like we wanted to add an option to completion but wanted to avoid having to pass it on every call to completion (vs doing it once at the beginning)?

we want to add options to codeAction creation logic. that would be in completion (with imports), in quick fixes, and refactoring. the common issues are for instance which quote to use for module names.
These options are more like formatting options, they should be provided once at the beginning, or when they change.

mhegazy · 2018-03-06T00:03:04Z

src/server/protocol.ts

@@ -2588,6 +2588,20 @@ namespace ts.server.protocol {
        insertSpaceBeforeTypeAnnotation?: boolean;
    }

+    export interface Options {
+        quote: "double" | "single";


these should be all be optional.

mhegazy · 2018-03-06T00:07:05Z

src/server/protocol.ts

         */
-        includeExternalModuleExports: boolean;
+        includeExternalModuleExports?: boolean;


not sure why should these be lumped with the other... i mean we can change to a global config system, but maybe on a separate change, and then we need to discuss that as well with the VSCode folks and with @amcasey

Spoke to @amcasey and seems like it will work for him. so i am fine with he change. we probably need to give them more descriptive names.

Wouldn't changing the name be a breaking change though? Although we could deprecate the existing name and add an equivalent one.

I think @mhegazy meant that we should give them more descriptive names in their new location. In their old location, they have to stay as-is for back-compat.

@amcasey Do you have some good suggestions for the new names?

I had envisioned that they would be in a nested object called (e.g.) "completions". However, if that's not idiomatic, incorporating "completions" into the name should be sufficient - e.g. "includeCompletionsForExternalModuleExports".

mhegazy · 2018-03-06T00:11:13Z

src/services/services.ts

            });
        }

-        function getCombinedCodeFix(scope: CombinedCodeFixScope, fixId: {}, formatOptions: FormatCodeSettings): CombinedCodeActions {
+        function getCombinedCodeFix(scope: CombinedCodeFixScope, fixId: {}, formatOptions: FormatCodeSettings, options: Options): CombinedCodeActions {


we aslo need the options for applicablerefactorings and organizeImports

mhegazy · 2018-03-06T20:38:07Z

tests/baselines/reference/api/typescript.d.ts

-        organizeImports(scope: OrganizeImportsScope, formatOptions: FormatCodeSettings): ReadonlyArray<FileTextChanges>;
+        getApplicableRefactors(fileName: string, positionOrRaneg: number | TextRange, options: Options): ApplicableRefactorInfo[];
+        getEditsForRefactor(fileName: string, formatOptions: FormatCodeSettings, positionOrRange: number | TextRange, refactorName: string, actionName: string, options: Options): RefactorEditInfo | undefined;
+        organizeImports(scope: OrganizeImportsScope, formatOptions: FormatCodeSettings, options: Options): ReadonlyArray<FileTextChanges>;


the new options parameter should be optional. right?

…todo: come up with better names)

RyanCavanaugh · 2018-03-06T22:45:50Z

src/services/types.ts

@@ -214,6 +214,14 @@ namespace ts {
        installPackage?(options: InstallPackageOptions): Promise<ApplyCodeActionCommandResult>;
    }

+    export interface Options {


I'd really prefer a more descriptive name

DanielRosenwasser · 2018-03-07T07:40:25Z

src/services/types.ts

@@ -214,6 +214,14 @@ namespace ts {
        installPackage?(options: InstallPackageOptions): Promise<ApplyCodeActionCommandResult>;
    }

+    export interface Options {
+        readonly quote?: "double" | "single";
+        readonly includeCompletionsForExternalModuleExports?: boolean;


includeCompletionsForModuleExports because nobody after 2015 should ever use the term "external module"

DanielRosenwasser · 2018-03-07T07:41:23Z

src/services/types.ts

+    export interface Options {
+        readonly quote?: "double" | "single";
+        readonly includeCompletionsForExternalModuleExports?: boolean;
+        readonly includeCompletionsWithInsertText?: boolean;


I'd prefer a better name here, but I'm not sure what.

DanielRosenwasser · 2018-03-07T07:42:40Z

tests/cases/fourslash/fourslash.ts

@@ -527,6 +522,13 @@ declare namespace FourSlashInterface {
        category: string;
        code: number;
    }
+    interface Options {
+        quote?: "double" | "single";
+        includeInsertTextCompletions?: boolean;


where's the other option here?

…18169)

DanielRosenwasser · 2018-03-14T21:16:53Z

src/server/protocol.ts

         */
-        includeInsertTextCompletions: boolean;
+        includeInsertTextCompletions?: boolean;


Looks like we have two separate names for the same concept (i.e. different thing in protocol vs services)

This is a deprecated property of CompletionsRequestArgs to support the old name, the new name is later on in this file.

DanielRosenwasser · 2018-03-14T21:23:15Z

src/server/protocol.ts

+         * For those entries, The `insertText` and `replacementSpan` properties will be set to change from `.x` property access to `["x"]`.
+         */
+        readonly includeCompletionsWithInsertText?: boolean;
+        readonly importModuleSpecifierPreference?: "relative" | "baseUrl";


We should reconsider if we want to call this something like "non-relative" just because most of the time, people are using this with path-mapping; but I think it really depends on the behavior. If we always give completions from the baseUrl (probably undesirable) then we can keep this as baseUrl. But if we already have this, it's probably fine. @mhegazy @sheetalkamat

DanielRosenwasser · 2018-03-14T21:25:24Z

src/server/protocol.ts

@@ -2633,6 +2633,21 @@ namespace ts.server.protocol {
        insertSpaceBeforeTypeAnnotation?: boolean;
    }

+    export interface Options {
+        readonly quote?: "double" | "single";


quotePreference

DanielRosenwasser · 2018-03-14T21:29:41Z

src/server/protocol.ts

@@ -2633,6 +2633,21 @@ namespace ts.server.protocol {
        insertSpaceBeforeTypeAnnotation?: boolean;
    }

+    export interface Options {


UserPreferences? Thoughts @RyanCavanaugh? (keep #22236 (comment) in mind)

mhegazy · 2018-05-15T20:44:31Z

These are exposed in VSCode settings:

Quote style:

"typescript.preferences.quoteStyle": "double"

or

"typescript.preferences.quoteStyle": "single"

module import relative vs non-relative:

"typescript.preferences.importModuleSpecifier": "relative"

or

"typescript.preferences.importModuleSpecifier": "non-relative"

ghost requested review from mhegazy, sheetalkamat and amcasey February 28, 2018 19:14

Support services settings

a40fab8

ghost force-pushed the services_settings branch from b47c0f3 to a40fab8 Compare February 28, 2018 22:31

sheetalkamat reviewed Mar 1, 2018

View reviewed changes

mhegazy reviewed Mar 1, 2018

View reviewed changes

amcasey reviewed Mar 2, 2018

View reviewed changes

ghost mentioned this pull request Mar 5, 2018

Support quote style in import fix #22342

Closed

ghost force-pushed the services_settings branch from e3aabe0 to f36bcd3 Compare March 5, 2018 21:29

Code review

590faf0

ghost force-pushed the services_settings branch from f36bcd3 to 590faf0 Compare March 5, 2018 21:43

mhegazy reviewed Mar 6, 2018

View reviewed changes

More review

4d592ee

ghost force-pushed the services_settings branch from eea8288 to 4d592ee Compare March 6, 2018 16:09

mhegazy reviewed Mar 6, 2018

View reviewed changes

Use different names for Options and GetCompletionsAtPositionOptions (…

327d688

…todo: come up with better names)

ghost force-pushed the services_settings branch from be34eb6 to 327d688 Compare March 6, 2018 21:32

mhegazy approved these changes Mar 6, 2018

View reviewed changes

RyanCavanaugh reviewed Mar 6, 2018

View reviewed changes

DanielRosenwasser requested changes Mar 7, 2018

View reviewed changes

More renaming

85b8abc

ghost force-pushed the services_settings branch from 1463a6a to b218afe Compare March 12, 2018 21:30

Support quote style in importFixes

c0e2d52

ghost force-pushed the services_settings branch from b218afe to c0e2d52 Compare March 12, 2018 21:31

Add importModuleSpecifierPreference option

e8033dc

ghost force-pushed the services_settings branch 2 times, most recently from beb6aef to ce9ddde Compare March 12, 2018 22:00

Merge branch 'master' into services_settings

940979a

ghost force-pushed the services_settings branch 3 times, most recently from 6525eb1 to 468d900 Compare March 12, 2018 23:24

Support quote style for throw new Error('Method not implemented.') (#…

ca0beaf

…18169)

ghost force-pushed the services_settings branch from 468d900 to ca0beaf Compare March 13, 2018 16:39

DanielRosenwasser requested changes Mar 14, 2018

View reviewed changes

Merge branch 'master' into services_settings

b06b0d8

ghost force-pushed the services_settings branch from f0d972c to d48a7d7 Compare March 15, 2018 19:24

options -> preferences

6e1adaf

ghost force-pushed the services_settings branch from d48a7d7 to 6e1adaf Compare March 15, 2018 20:15

mjbvz mentioned this pull request Mar 16, 2018

Support TS options in quick fixes/refactoring microsoft/vscode#45948

Closed

Merge branch 'master' into services_settings

abc1000

DanielRosenwasser approved these changes Mar 20, 2018

View reviewed changes

ghost merged commit cc39213 into master Mar 20, 2018

ghost deleted the services_settings branch March 20, 2018 20:08

This was referenced Mar 29, 2018

Support options in quick fixes/refactoring #20619

Closed

autoimport should care about semicolons #19882

Closed

Add file extensions when auto-completing import paths #19019

Closed

alextreppass mentioned this pull request May 15, 2018

auto import always uses relative (to the current file) module paths in presense of baseUri #21860

Open

microsoft locked and limited conversation to collaborators Jul 31, 2018

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support services settings #22236

Support services settings #22236

ghost commented Feb 28, 2018 •

edited by ghost

Loading

amcasey commented Mar 1, 2018

sheetalkamat Mar 1, 2018

ghost Mar 1, 2018 •

edited by ghost

Loading

mhegazy Mar 1, 2018 •

edited

Loading

ghost Mar 1, 2018

mhegazy Mar 1, 2018

mhegazy Mar 1, 2018

mhegazy Mar 1, 2018

mhegazy commented Mar 1, 2018

ghost commented Mar 1, 2018

mhegazy commented Mar 1, 2018

amcasey commented Mar 2, 2018

amcasey Mar 2, 2018

amcasey Mar 2, 2018

mhegazy commented Mar 6, 2018

mhegazy Mar 6, 2018

mhegazy Mar 6, 2018

mhegazy Mar 6, 2018

ghost Mar 6, 2018

amcasey Mar 6, 2018

ghost Mar 6, 2018

amcasey Mar 6, 2018

mhegazy Mar 6, 2018

mhegazy Mar 6, 2018 •

edited

Loading

RyanCavanaugh Mar 6, 2018

DanielRosenwasser Mar 7, 2018

DanielRosenwasser Mar 7, 2018

DanielRosenwasser Mar 7, 2018

DanielRosenwasser Mar 14, 2018

ghost Mar 15, 2018

DanielRosenwasser Mar 14, 2018

DanielRosenwasser Mar 14, 2018

DanielRosenwasser Mar 14, 2018

mhegazy commented May 15, 2018

Support services settings #22236

Support services settings #22236

Conversation

ghost commented Feb 28, 2018 • edited by ghost Loading

amcasey commented Mar 1, 2018

Choose a reason for hiding this comment

ghost Mar 1, 2018 • edited by ghost Loading

Choose a reason for hiding this comment

mhegazy Mar 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhegazy commented Mar 1, 2018

ghost commented Mar 1, 2018

mhegazy commented Mar 1, 2018

amcasey commented Mar 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhegazy commented Mar 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhegazy Mar 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhegazy commented May 15, 2018

Quote style:

module import relative vs non-relative:

ghost commented Feb 28, 2018 •

edited by ghost

Loading

ghost Mar 1, 2018 •

edited by ghost

Loading

mhegazy Mar 1, 2018 •

edited

Loading

mhegazy Mar 6, 2018 •

edited

Loading