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

Add 'disableSuggestions' to UserPreferences #23283

Merged
5 commits merged into from
Apr 10, 2018
Merged

Add 'disableSuggestions' to UserPreferences #23283

5 commits merged into from
Apr 10, 2018

Conversation

ghost
Copy link

@ghost ghost commented Apr 9, 2018

Fixes #23177

@ghost ghost requested a review from sheetalkamat April 9, 2018 20:25
@ghost
Copy link
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
Member

looking

@sheetalkamat
Copy link
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
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
Author

ghost commented Apr 9, 2018

@sheetalkamat Good to go?

@@ -398,14 +398,14 @@ namespace ts.server {
if (!this.formatSettings) {
this.formatSettings = getDefaultFormatCodeSettings(this.host);
Copy link
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
Author

Choose a reason for hiding this comment

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

Done.

assign(this.formatSettings, formatSettings);
}
else {
this.formatSettings = mergeMapLikes(this.formatSettings, formatSettings);
Copy link
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
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.

None yet

2 participants