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

Introduce LS setting to delay diagnostic publishing to idle #4612

Merged
merged 3 commits into from
Aug 9, 2018
Merged

Introduce LS setting to delay diagnostic publishing to idle #4612

merged 3 commits into from
Aug 9, 2018

Conversation

MikhailArkhipov
Copy link

@@ -28,6 +28,8 @@ public class PythonAnalysisOptions {
public string[] information { get; } = Array.Empty<string>();
public string[] disabled { get; } = Array.Empty<string>();

public int diagnosticPublishDelay = 1000;
Copy link
Member

@zooba zooba Aug 8, 2018

Choose a reason for hiding this comment

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

Not sure how I feel about having a setting here that isn't actually used by Server itself. Wouldn't it be better to delay at the actual point of publishing anyway? (And maybe default to "no delay" but then make the VSC configuration default to 1s?)

Copy link
Author

Choose a reason for hiding this comment

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

These are LanguageServerSettings they are only used in the LS scenario. They happen to be defined at the Server level since it was simpler. I can try pushing them to the LS instead.

Copy link
Author

Choose a reason for hiding this comment

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

Generally speaking the setting may come useful, the complaint was not VSC specific. IT was more about "I don't want to see squiggles as I type, I know the construct is incomplete". Applies to VS too. openFilesOnly is already there too.

Now, handling is in VSC LS side and does not affect anything else. BTW, don't we merge everything into LS at some point anyway?

Copy link
Member

Choose a reason for hiding this comment

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

VS already throttles on the editor side and on the error list (though the latter may go away soon when we move to the proper table API), which is why we don't see this complaint so often. We don't really need to also throttle on sending the diagnostics, but it may be confusing later on if the setting doesn't appear to do what it claims to.

(And eventually we'll use LSP for both, but I expect it'll be a different adapter for VS that uses the service host support, rather than being a standalone executable. So the DLLs will be shared, but not necessarily the EXE.)

Copy link
Author

Choose a reason for hiding this comment

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

To summarize, throttling is happening at the LS side, not on the Server side. LanguageServerSettings declared in Server, but it does contain fields specific to LS as it was simpler to implement conditions in the Server code. openFiilesOnly comes to mind. I've spent some time trying to move LanguageServerSettings to LS and just use Server methods, but it gets complicated pretty soon.

Copy link
Author

Choose a reason for hiding this comment

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

It is possible to split class into ServerSettings and LanguageServerSettings and derive one from another so Server will see less.

@MikhailArkhipov MikhailArkhipov merged commit 5d4e502 into microsoft:master Aug 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants