LSP: New settings option to Disable server and Better Goto Definition for property and file paths (1/2) #2504

wolmir · 2022-09-29T19:48:51Z

This PR will allow the Goto Definition for feature for properties inside parameter files, and also for files referenced in the dvc.yaml.

It will also introduce an extension setting to disable the language server when needed.

Demo:

Screen.Recording.2022-10-04.at.14.15.38.mov

codeclimate · 2022-10-04T18:01:31Z

Code Climate has analyzed commit 92e616f and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 88.4% (85% is the threshold).

This pull request will bring the total coverage in the repository to 96.6% (-0.2% change).

View more on Code Climate.

mattseddon · 2022-10-05T09:07:30Z

Needs #2530 to be ready before merging.

sroy3 · 2022-10-05T13:55:43Z

extension/src/lspClient/languageClient.ts

+          this.isRunning() &&
+          this.languageServerDisabled()
+        ) {
+          this.stop()


Can we dispose of the onDidChangeConfiguration here? We could get rid of the this.isRunning() && this.languageServerDisabled() as well as this would only run once (when disabling the server the first time).

sroy3 · 2022-10-05T14:02:39Z

languageServer/src/languageHelpers/baseLanguageHelper.ts

+
+  private findLocationsForPropertySymbol(symbol: DocumentSymbol) {
+    const propertyPath = symbol.detail
+    const itIsHere = propertyPath && this.hasProperty(propertyPath)


Nitpick, but is it possible to rename isHere as it follows our boolean naming conventions more.

Yes, thanks!

sroy3 · 2022-10-05T14:10:08Z

languageServer/src/languageHelpers/yamlHelper.ts

+  visit
+} from 'yaml'
+import { BaseLanguageHelper } from './baseLanguageHelper'
+import * as RegExes from './regexes'


Since you can still import the regexes individually (ex.: filePaths), I think it'd be better to rename the exported constants in regexes.ts to identify them as regexes and simply import them individually. The other solution would be to wrap them all in a regexes object and export that instead, but I think the first solution is better.

Yes, that's what I tried first, but I worried if anyone would be annoyed about typing a prefix every time they create/use the constant.
But now that I'm writing this I realised that typing Regexes.something is exactly the same thing and now I'm feeling really dumb.
I'll change it, thanks!

sroy3 · 2022-10-05T14:13:34Z

languageServer/src/languageHelpers/yamlHelper.ts

+  }
+
+  protected toJSON(): unknown {
+    return this.rootNode.toJS()


Why toJS() instead of toJSON()? Might be worth a comment.

Makes more sense, thanks!

sroy3 · 2022-10-05T14:15:54Z

languageServer/src/regexes.ts

@@ -1,3 +1,4 @@
 export const variableTemplates = /\${([^}]+)}/g
 export const filePaths = /[\d/A-Za-z]+\.[A-Za-z]+/g
 export const alphadecimalWords = /[\dA-Za-z]+/g
+export const propertyPathLike = /[\d.A-Z[\]a-z]+/g


Any differences between this file and languageServer/src/languageHelpers/regexes.ts? They should be merged if so. Either way we should only have one file only for regexes.

Forgot about it, will delete it 👍

mattseddon · 2022-10-05T21:00:26Z

extension/src/lspClient/languageClient.ts

-    const textDocuments = files.map(filePath => {
-      const uri = Uri.file(filePath).toString()
-      const languageId = filePath.endsWith('yaml') ? 'yaml' : 'json'
-      const text = readFileSync(filePath, 'utf8')


I was very confused as to how the "ring-fenced" language server managed to bring down the extension host.

This would be what caused the crashes. A synchronous call to read every file would definitely bring down the thread.

The watcher would not have caused this issue. We already have at least one watcher which watches the entire workspace (and more).

The more I think about this the more I feel like the approach was backwards. We should be parsing each dvc.yaml file when it is opened and checking to see which symbols are files. From there we read those files so that we can jump into them if needed.

Relevant: https://code.visualstudio.com/updates/v1_72#_improved-startup-performance

See "NO MORE SYNC FS CALLS"

julieg18 · 2022-10-05T21:14:38Z

extension/src/lspClient/languageClient.ts

-    await this.client.sendRequest('initialTextDocuments', {
-      textDocuments
-    })
+      workspace.onDidChangeConfiguration((event: ConfigurationChangeEvent) => {


If the language server is on and I disable it in the settings, the server turns off without any need to reload. But if I try to turn it back on in settings, I need to reload the page to see the language server running again:

Screen.Recording.2022-10-05.at.4.13.12.PM.mov

Do we want it to be able to turn it back on without needing to reload?

julieg18 · 2022-10-05T21:52:14Z

languageServer/src/languageHelpers/yamlHelper.ts

+import * as RegExes from './regexes'
+
+export class YamlHelper extends BaseLanguageHelper<Document> {
+  protected rootNode!: Document


Never seen a ! used like this before! Learned something new today :)

julieg18 · 2022-10-05T22:36:42Z

languageServer/src/LanguageServer.ts

@@ -10,16 +10,14 @@ import {
  Location,


File definitions inside of cmds don't seem to work correctly on my end:

Screen.Recording.2022-10-05.at.5.35.47.PM.mov

julieg18 · 2022-10-05T22:40:11Z

languageServer/src/LanguageServer.ts

@@ -10,16 +10,14 @@ import {
  Location,
  Position,


What should be shown if a file hasn't been opened yet? Currently, it appears to take us somewhere that doesn't seem to make sense:

Screen.Recording.2022-10-05.at.5.39.18.PM.mov

mattseddon · 2022-10-05T21:49:41Z

extension/src/lspClient/languageClient.ts

-    await this.client.sendRequest('initialTextDocuments', {
-      textDocuments
-    })
+      workspace.onDidChangeConfiguration((event: ConfigurationChangeEvent) => {


[I] this is a disposable and should be tracked.

mattseddon · 2022-10-05T21:50:54Z

extension/src/lspClient/languageClient.ts

@@ -68,6 +54,14 @@ export class LanguageClientWrapper extends Disposable {
    this.client.stop()


[Q] Are you sure that stopping the client stops the server?

mattseddon · 2022-10-05T21:56:00Z

languageServer/src/languageHelpers/baseLanguageHelper.ts

@@ -0,0 +1,105 @@
+import { has } from 'lodash'


Should be using lodash subpackages for consistency with the rest of the app.

mattseddon · 2022-10-05T23:55:09Z

languageServer/src/LanguageServer.ts

@@ -94,18 +61,33 @@ export class LanguageServer {
    return new TextDocumentWrapper(doc)
  }

+  private isPossibleFilePath(symbol: DocumentSymbol) {
+    const isFile = symbol.kind === SymbolKind.File


[Q] any ideas how symbol.kind is set to SymbolKind.File?

mattseddon · 2022-10-06T00:19:03Z

languageServer/src/languageHelpers/baseLanguageHelper.ts

+  findLocationsFor(symbol: DocumentSymbol): Location[]
+}
+
+export abstract class BaseLanguageHelper<RootNode> implements LanguageHelper {


It's usual practice in the codebase to have this a <T>. I was looking for where <RootNode> is defined for a couple of minutes.

mattseddon · 2022-10-06T00:22:36Z

languageServer/src/languageHelpers/baseLanguageHelper.ts

+  SymbolKind
+} from 'vscode-languageserver/node'
+
+export interface LanguageHelper {


[N] Probably don't need this interface on top of the abstract class.

mattseddon · 2022-10-06T00:38:13Z

extension/src/lspClient/languageClient.ts

-    const textDocuments = files.map(filePath => {
-      const uri = Uri.file(filePath).toString()
-      const languageId = filePath.endsWith('yaml') ? 'yaml' : 'json'
-      const text = readFileSync(filePath, 'utf8')


The more I think about this the more I feel like the approach was backwards. We should be parsing each dvc.yaml file when it is opened and checking to see which symbols are files. From there we read those files so that we can jump into them if needed.

mattseddon · 2022-10-06T00:50:10Z

languageServer/src/TextDocumentWrapper.ts

@@ -45,132 +33,11 @@ export class TextDocumentWrapper implements ITextDocumentWrapper {
    return parseDocument(this.getText())


getYamlDocument is no longer used anywhere (seems like it has been moved into the languageHelper) it should be removed from this class and the interface.

wolmir force-pushed the dvc-lsp-definition-improvements branch from 5072476 to acdb4fe Compare October 3, 2022 12:26

wolmir added 7 commits October 4, 2022 11:01

Make some readability improvements

7b7bc26

Add config option to disable the language server

99a19c5

Add tests for file paths in dvc.yamls

8a05a44

Add missing assertion to test

a3c6ad8

Fix Goto definition for file paths

dd79bb3

Add a test for composite file paths

aeb6532

Add more tests

ca424a1

wolmir force-pushed the dvc-lsp-definition-improvements branch from d962189 to ca424a1 Compare October 4, 2022 14:02

Fix property Regexes

4863de8

wolmir changed the title ~~[WIP] Dvc lsp definition improvements~~ LSP: Disable server settings and Better Goto Definition for property and file paths Oct 4, 2022

Fix windows test

257daf6

wolmir changed the title ~~LSP: Disable server settings and Better Goto Definition for property and file paths~~ LSP: New settings option to Disable server and Better Goto Definition for property and file paths Oct 4, 2022

fix windows tests again

92e616f

wolmir changed the title ~~LSP: New settings option to Disable server and Better Goto Definition for property and file paths~~ LSP: New settings option to Disable server and Better Goto Definition for property and file paths (1/2) Oct 4, 2022

wolmir marked this pull request as ready for review October 4, 2022 18:03

wolmir requested review from mattseddon, sroy3 and julieg18 as code owners October 4, 2022 18:03

shcheklein assigned wolmir Oct 4, 2022

mattseddon added the do not merge label Oct 5, 2022

sroy3 reviewed Oct 5, 2022

View reviewed changes

mattseddon reviewed Oct 5, 2022

View reviewed changes

julieg18 reviewed Oct 5, 2022

View reviewed changes

mattseddon reviewed Oct 6, 2022

View reviewed changes

mattseddon marked this pull request as draft January 30, 2023 04:11

mattseddon closed this Jan 30, 2023

mattseddon deleted the dvc-lsp-definition-improvements branch January 30, 2023 20:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

LSP: New settings option to Disable server and Better Goto Definition for property and file paths (1/2) #2504

LSP: New settings option to Disable server and Better Goto Definition for property and file paths (1/2) #2504

wolmir commented Sep 29, 2022 •

edited

Loading

codeclimate bot commented Oct 4, 2022

mattseddon commented Oct 5, 2022

sroy3 Oct 5, 2022

wolmir Oct 5, 2022

sroy3 Oct 5, 2022

wolmir Oct 5, 2022

sroy3 Oct 5, 2022

wolmir Oct 5, 2022

sroy3 Oct 5, 2022

wolmir Oct 5, 2022

sroy3 Oct 5, 2022

wolmir Oct 5, 2022

mattseddon Oct 5, 2022

mattseddon Oct 5, 2022

mattseddon Oct 6, 2022

mattseddon Oct 7, 2022

julieg18 Oct 5, 2022

julieg18 Oct 5, 2022

julieg18 Oct 5, 2022

julieg18 Oct 5, 2022

mattseddon Oct 5, 2022

mattseddon Oct 5, 2022

mattseddon Oct 5, 2022

mattseddon Oct 5, 2022

mattseddon Oct 6, 2022

mattseddon Oct 6, 2022

mattseddon Oct 6, 2022

mattseddon Oct 6, 2022

		@@ -68,6 +54,14 @@ export class LanguageClientWrapper extends Disposable {
		this.client.stop()

		@@ -45,132 +33,11 @@ export class TextDocumentWrapper implements ITextDocumentWrapper {
		return parseDocument(this.getText())

LSP: New settings option to Disable server and Better Goto Definition for property and file paths (1/2) #2504

LSP: New settings option to Disable server and Better Goto Definition for property and file paths (1/2) #2504

Conversation

wolmir commented Sep 29, 2022 • edited Loading

codeclimate bot commented Oct 4, 2022

mattseddon commented Oct 5, 2022

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

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

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

wolmir commented Sep 29, 2022 •

edited

Loading