Skip to content

Add 'disableSuggestions' to UserPreferences#23283

Merged
5 commits merged into
masterfrom
disableSuggestions
Apr 10, 2018
Merged

Add 'disableSuggestions' to UserPreferences#23283
5 commits merged into
masterfrom
disableSuggestions

Conversation

@ghost
Copy link
Copy Markdown

@ghost ghost commented Apr 9, 2018

Fixes #23177

@ghost ghost requested a review from sheetalkamat April 9, 2018 20:25
@ghost
Copy link
Copy Markdown
Author

ghost commented Apr 9, 2018

@sheetalkamat If I comment out the new test, the failures in the other tsserverProjectSystem tests go away -- any idea how these are effecting each other?

@sheetalkamat
Copy link
Copy Markdown
Member

looking

@sheetalkamat
Copy link
Copy Markdown
Member

sheetalkamat commented Apr 9, 2018

@Andy-MS this is i think because of https://github.com/Microsoft/TypeScript/blob/master/src/server/editorServices.ts#L1849
It modifies host.preferences which could be defaultPreferences object which gets modified (https://github.com/Microsoft/TypeScript/blob/master/src/server/editorServices.ts#L452) Which your test modifies to skip the suggestion diag.. So any tests running after your test get that default preference = disabledSuggestion as true breaking those

@ghost
Copy link
Copy Markdown
Author

ghost commented Apr 9, 2018

Ugh, every property in UserPreferences is readonly, but readonly modifiers truly are a joke. #13002
Thanks for debugging that!

@ghost
Copy link
Copy Markdown
Author

ghost commented Apr 9, 2018

@sheetalkamat Good to go?

Comment thread src/server/scriptInfo.ts
@@ -398,14 +398,14 @@ namespace ts.server {
if (!this.formatSettings) {
this.formatSettings = getDefaultFormatCodeSettings(this.host);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This function returns a copy, so it is ok to modify .. So no need to clone unlike defaultPreferences... I am not sure how many times we call these configure function to say if its ok to clone the object (esp with such a big set of properties)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Done.

Comment thread src/server/scriptInfo.ts Outdated
assign(this.formatSettings, formatSettings);
}
else {
this.formatSettings = mergeMapLikes(this.formatSettings, formatSettings);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

is not this just:

this.formatSettings = { ... this.formatSettings, ...formatSettings};

@ghost ghost merged commit 4b706fc into master Apr 10, 2018
@ghost ghost deleted the disableSuggestions branch April 10, 2018 18:53
@ghost
Copy link
Copy Markdown
Author

ghost commented Apr 10, 2018

CC @mjbvz This should be added as a vscode setting.

@microsoft microsoft locked and limited conversation to collaborators Jul 25, 2018
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants