LSP unit testing of completions #1519
Conversation
| @@ -445,7 +445,11 @@ function TSHost(compilationSettings: CompilerOptions, initialFileNames: string[] | |||
| } | |||
|
|
|||
| function TSService(projectURL = "./") { | |||
| const logger = console | |||
| const logger = { | |||
There was a problem hiding this comment.
If we need to make vscode-jsonrpc work properly, we cannot print redundant logs in the output.
| @@ -358,7 +358,7 @@ export function logTiming<R, A extends unknown[]>(name: string, fn: (...args: A) | |||
| const start = performance.now(), | |||
| result = fn(...args), | |||
| end = performance.now(); | |||
| console.log(`${name.padStart(32)}${(end - start).toFixed(2).padStart(8)}ms`) | |||
| // console.log(`${name.padStart(32)}${(end - start).toFixed(2).padStart(8)}ms`) | |||
There was a problem hiding this comment.
Perhaps we can add a parameter or a global logger variable.
There was a problem hiding this comment.
Yes, I think so. A reasonable approach might be to add an LSP command-line parameter something like --log server.log that redirects all logs to there, and the default behavior remains going to console or connection.console.
Actually, if we can use connection.console always, presumably that can be intercepted? That seems like a cleaner solution.
There was a problem hiding this comment.
Indeed, having a universal behavior would indeed be better for interception. The problem here is that the required connection cannot be obtained.
This pull request will also address this problem.
| @@ -113,6 +115,7 @@ const diagnosticsDelay = 16; // ms delay for primary updated file | |||
| const diagnosticsPropagationDelay = 100; // ms delay for other files | |||
|
|
|||
| connection.onInitialize(async (params: InitializeParams) => { | |||
| connection.console.log(`Initializing civet server at ${new Date().toLocaleTimeString()}`); | |||
There was a problem hiding this comment.
All logs on the server should use the console on the connection as much as possible.
| const baseDir = params.workspaceFolders?.[0]?.uri.toString() | ||
| if (!baseDir) | ||
| throw new Error("Could not initialize without workspace folders") | ||
| if (!params.rootUri) { |
There was a problem hiding this comment.
Although rootUri has been deprecated, it is not difficult for us to support it.
There was a problem hiding this comment.
Is this for compatibility with tools other than VSCode (e.g. tester or Vim) that aren't updated to the new interface? Otherwise, I don't see why we'd want to support a deprecated interface.
There was a problem hiding this comment.
In the future, there may be a demand for Monaco applied in browsers. For example, directly integrating civet-related language services in it. However, the version of Monaco will be relatively outdated. Maintaining compatibility will be very valuable here.
We can build some better online playground. Honestly, I think civet's work on types is very constructive. However, I didn't feel this in civet's playground. I think this is very regrettable.
Also, regarding unit tests, it seems that workspaceFolders are not needed. Here, I think the cost is not high and it can be simply supported.
|
|
||
| require := createRequire import.meta.url | ||
|
|
||
| __dirname := import.meta.url |> .slice 7 |> dirname |
There was a problem hiding this comment.
I'd rather use url.fileURLToPath than .slice 7.
| @@ -358,7 +358,7 @@ export function logTiming<R, A extends unknown[]>(name: string, fn: (...args: A) | |||
| const start = performance.now(), | |||
| result = fn(...args), | |||
| end = performance.now(); | |||
| console.log(`${name.padStart(32)}${(end - start).toFixed(2).padStart(8)}ms`) | |||
| // console.log(`${name.padStart(32)}${(end - start).toFixed(2).padStart(8)}ms`) | |||
There was a problem hiding this comment.
Yes, I think so. A reasonable approach might be to add an LSP command-line parameter something like --log server.log that redirects all logs to there, and the default behavior remains going to console or connection.console.
Actually, if we can use connection.console always, presumably that can be intercepted? That seems like a cleaner solution.
| const baseDir = params.workspaceFolders?.[0]?.uri.toString() | ||
| if (!baseDir) | ||
| throw new Error("Could not initialize without workspace folders") | ||
| if (!params.rootUri) { |
There was a problem hiding this comment.
Is this for compatibility with tools other than VSCode (e.g. tester or Vim) that aren't updated to the new interface? Otherwise, I don't see why we'd want to support a deprecated interface.
| @@ -288,7 +294,7 @@ connection.onCompletion(async ({ textDocument, position, context: _context }) => | |||
| // Don't map for files that don't have a sourcemap (plain .ts for example) | |||
| if (sourcemapLines) { | |||
| position = forwardMap(sourcemapLines, position) | |||
| console.log('remapped') | |||
| // console.log('remapped') | |||
There was a problem hiding this comment.
It's fine to delete these rather than comment them out. Or log them to connection, possibly gated by a --verbose setting if feeling ambitious.
There was a problem hiding this comment.
Yes, what you said is correct. I do want to add a --verbose parameter. The work here is just relatively simple brute-force commenting.
In the past two days, I mainly want to make the unit tests run. In the next work, I will improve what you said and what I am going to do.
|
Thanks for digging into this, the LSP definitely needs all the help it can get! 👏 |
No description provided.