Add: Workspace symbols #510

aspeddro · 2022-07-22T23:32:22Z

Close #292

server/src/utils.ts

server/src/server.ts

cristianoc · 2022-07-23T01:05:52Z

analysis/src/Symbols.ml

+         symbol)
+
+let workspace ~dir =
+  let open FindFiles in


There are a bunch of places in the codebase with this kind of code.
Wondering if it's similar enough to grant some refactoring.
Or if the differences are too big to gain something.

E.g. the functionality to read sourceDirectories could be factored out.

Should we add a separate issue to track this @cristianoc ?

I think this can be assessed now, and any follow-on work postponed.

@aspeddro will you have a look at this? After that, this should be good for merging.

EDIT: Sorry, this + the other comments about the tests that @cristianoc had.

analysis/tests/src/expected/DocumentSymbol.res.txt

analysis/src/Commands.ml

cristianoc · 2022-07-23T01:10:08Z

No tests for the new command yet right?

analysis/tests/workspace-tests.txt

analysis/tests/workspace/expected/workspaceSymbol.txt

analysis/tests/test.sh

cristianoc · 2022-07-24T06:30:28Z

server/src/utils.ts

-export let runAnalysis = (args: Array<any>, binaryPath?: string, cwd?: string) => {
-  let analysisPath = binaryPath != null ? binaryPath : getAnalysisBinaryPath()
+export let runAnalysis = (args: Array<any>, cwd?: string) => {
+  let binaryPath = getAnalysisBinaryPath()


cristianoc · 2022-07-24T06:33:55Z

@zth not necessarily about this PR, but we should consider beginning to add tests for the TypeScript part of the extension. That's where most breakages have been recently (and where we have no tests).
Instead of going top down, test framework, overhead, unclear utility, i.e. using "best practices". How about doing a quick review of recent breakages and add the least amount of infra required to prevent those from re-occurring. Then build out from there in future.

zth · 2022-07-24T16:06:45Z

@zth not necessarily about this PR, but we should consider beginning to add tests for the TypeScript part of the extension. That's where most breakages have been recently (and where we have no tests). Instead of going top down, test framework, overhead, unclear utility, i.e. using "best practices". How about doing a quick review of recent breakages and add the least amount of infra required to prevent those from re-occurring. Then build out from there in future.

Sounds good to me. Let's discuss this separately then.

server/src/server.ts

aspeddro · 2022-07-24T20:52:54Z

server/src/server.ts

@@ -4,14 +4,14 @@ import * as v from "vscode-languageserver";
 import * as rpc from "vscode-jsonrpc/node";
 import * as path from "path";
 import fs from "fs";
-import os from "os";


os is not used

aspeddro · 2022-07-24T21:49:52Z

package.json

@@ -179,7 +188,7 @@
 		"vscode:prepublish": "npm run clean && npm run compile",
 		"compile": "tsc -b",
 		"watch": "tsc -b -w",
-		"postinstall": "cd server && npm i && cd ../client && npm i && cd ../analysis/tests && npm i && cd ../reanalyze/examples/deadcode && npm i && cd ../termination && npm i"
+		"postinstall": "cd server && npm i && cd ../client && npm i && cd ../analysis/reanalyze/examples/deadcode && npm i && cd ../termination && npm i"


Removed cd analysis/tests && npm i. No longer has a package.json file

aspeddro · 2022-07-24T22:05:17Z

There were the following changes in the tests folder:

tests/src/, tests/not_compiled, tests/package.json and tests/bsconfig.json moved to tests/document/ folder.

cristianoc · 2022-07-25T03:23:36Z

The last changes slowed down tests by about 3 seconds each clean build+test.
10 features would slow down 30 seconds, so looking into a more lightweight approach

cristianoc · 2022-07-25T03:23:51Z

Let me test something.

cristianoc · 2022-07-25T03:27:03Z

Oops commit history is gone.
Anyhow, it's just about putting tests into analysis/tests/src/workspace and change nothing in the structure.

cristianoc · 2022-07-25T03:27:40Z

This way we also avoid to merge conflict any other PR currently ongoing.

cristianoc · 2022-07-25T03:29:33Z

The last changes slowed down tests by about 3 seconds each clean build+test.
10 features would slow down 30 seconds, so looking into a more lightweight approach

Even if that count is inaccurate, we don't want those 3 seconds.
If anything, faster would be better.

aspeddro · 2022-07-25T17:07:30Z

Anyhow, it's just about putting tests into analysis/tests/src/workspace and change nothing in the structure.

Placing tests in analysis/tests/src/workspace will get all symbols in analysis/tests/src/. The workspaceSymbols command needs to read bsconfig.json and get sources.

When putting in tests/workspaces I isolate the files in analysis/tests/src

cristianoc · 2022-07-25T17:43:16Z

Anyhow, it's just about putting tests into analysis/tests/src/workspace and change nothing in the structure.

Placing tests in analysis/tests/src/workspace will get all symbols in analysis/tests/src/. The workspaceSymbols command needs to read bsconfig.json and get sources.

When putting in tests/workspaces I isolate the files in analysis/tests/src

Reading bsconfig is always tricky, as there are a bunch of rules, and might change in future.
For example, global rename does not look into bsconfig.

Related question: what about dependencies?

About tests: there's quite a lot of freedom how test commands can be structured, and surely one can pass a folder as parameter or some other restriction to avoid noisy results.

cristianoc · 2022-07-26T03:58:15Z

Making this run on CI: #519

cristianoc reviewed Jul 23, 2022

View reviewed changes

server/src/utils.ts Outdated Show resolved Hide resolved

cristianoc reviewed Jul 23, 2022

View reviewed changes

server/src/server.ts Show resolved Hide resolved

cristianoc reviewed Jul 23, 2022

View reviewed changes

analysis/tests/src/expected/DocumentSymbol.res.txt Show resolved Hide resolved

cristianoc reviewed Jul 23, 2022

View reviewed changes

analysis/src/Commands.ml Show resolved Hide resolved

aspeddro commented Jul 23, 2022

View reviewed changes

analysis/tests/workspace-tests.txt Outdated Show resolved Hide resolved

aspeddro commented Jul 23, 2022

View reviewed changes

analysis/tests/workspace/expected/workspaceSymbol.txt Outdated Show resolved Hide resolved

cristianoc reviewed Jul 24, 2022

View reviewed changes

analysis/tests/test.sh Outdated Show resolved Hide resolved

cristianoc reviewed Jul 24, 2022

View reviewed changes

zth reviewed Jul 24, 2022

View reviewed changes

server/src/server.ts Outdated Show resolved Hide resolved

aspeddro commented Jul 24, 2022

View reviewed changes

aspeddro marked this pull request as draft July 24, 2022 21:07

aspeddro commented Jul 24, 2022

View reviewed changes

aspeddro marked this pull request as ready for review July 24, 2022 21:50

add workspaceSymbols

b5c13b0

aspeddro force-pushed the workspace-symbols branch from 181e056 to b5c13b0 Compare July 24, 2022 22:27

cristianoc added 3 commits July 26, 2022 05:22

Speed up test.

4f85012

Try test with parallelism in CI.

9735bd0

Move one dir up.

750d37d

Actually check the expectation files.

9a89ca7

cristianoc mentioned this pull request Jul 26, 2022

Ws symbols #519

Closed

cristianoc mentioned this pull request Jul 26, 2022

Ws symbols (continues from #510) #520

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add: Workspace symbols #510

Add: Workspace symbols #510

aspeddro commented Jul 22, 2022

cristianoc Jul 23, 2022

zth Jul 24, 2022

cristianoc Jul 24, 2022

zth Jul 25, 2022 •

edited

Loading

cristianoc commented Jul 23, 2022

cristianoc Jul 24, 2022

cristianoc commented Jul 24, 2022

zth commented Jul 24, 2022

aspeddro Jul 24, 2022

aspeddro Jul 24, 2022

aspeddro commented Jul 24, 2022

cristianoc commented Jul 25, 2022

cristianoc commented Jul 25, 2022

cristianoc commented Jul 25, 2022 •

edited

Loading

cristianoc commented Jul 25, 2022

cristianoc commented Jul 25, 2022

aspeddro commented Jul 25, 2022

cristianoc commented Jul 25, 2022

cristianoc commented Jul 26, 2022

Add: Workspace symbols #510

Are you sure you want to change the base?

Add: Workspace symbols #510

Conversation

aspeddro commented Jul 22, 2022

cristianoc Jul 23, 2022

Choose a reason for hiding this comment

zth Jul 24, 2022

Choose a reason for hiding this comment

cristianoc Jul 24, 2022

Choose a reason for hiding this comment

zth Jul 25, 2022 • edited Loading

Choose a reason for hiding this comment

cristianoc commented Jul 23, 2022

cristianoc Jul 24, 2022

Choose a reason for hiding this comment

cristianoc commented Jul 24, 2022

zth commented Jul 24, 2022

aspeddro Jul 24, 2022

Choose a reason for hiding this comment

aspeddro Jul 24, 2022

Choose a reason for hiding this comment

aspeddro commented Jul 24, 2022

cristianoc commented Jul 25, 2022

cristianoc commented Jul 25, 2022

cristianoc commented Jul 25, 2022 • edited Loading

cristianoc commented Jul 25, 2022

cristianoc commented Jul 25, 2022

aspeddro commented Jul 25, 2022

cristianoc commented Jul 25, 2022

cristianoc commented Jul 26, 2022

zth Jul 25, 2022 •

edited

Loading

cristianoc commented Jul 25, 2022 •

edited

Loading