Use symbol provider from new language server to place code lenses in tests. #2456

ericsnowcurrently · 2018-08-28T20:22:57Z

(fixes #1948)

Title summarizes what is changing
Has a news entry file (remember to thank yourself!)
Has unit tests & system/integration tests
~~Any new/changed dependencies in package.json are pinned (e.g. "1.2.3", not "^1.2.3" for the specified version)~~
~~package-lock.json has been regenerated by running npm install (if dependencies have changed)~~

This is a derivative of @DonJayamanne's #2197. Differences:

removes extra Jedi-related changes
PythonSymbolProvider -> JediSymbolProvider
SymbolProvider -> LanguageServerSymbolProvider (and moved to src/client/providers/symbolProvider.ts)
flattens the symbol data returned from the language server
works around a bug in the language server (off-by-one kind)
adds tests for LanguageServerSymbolProvider

ericsnowcurrently · 2018-08-29T21:24:39Z

FWIW, I don't see any difference with this PR applied locally. Code lenses are not showing up (whereas they do for Jedi)...

ericsnowcurrently · 2018-08-29T21:30:21Z

I used the repro case detailed in #1948 (comment).

ericsnowcurrently · 2018-08-30T00:09:12Z

Looks like the data returned from the language server doesn't work quite right. I'll open an issue (or two) as soon as I can. I'm also going to work around the problem for now.

ericsnowcurrently · 2018-08-30T21:31:10Z

I've opened Language Server issue for things I've had to work around:

Other than that, I had to flatten the data returned from the language server. :)

d3r3kk

You need a news file, and other than that maybe have a look at the comments made on the recursive routine, I'd love to see us not create new objects with each iteration and copy them (and their members) about. If we can get rid of the call .push(...flattenedChild) and instead push each flattened child into the list as it progresses that might help with perf/memory on larger projects.

d3r3kk · 2018-08-30T22:02:23Z

src/client/providers/symbolProvider.ts

+}
+
+function flattenSymbolTree(tree: ILSSymbolTree, uri: Uri, containerName: string = ''): SymbolInformation[] {
+    const flattened: SymbolInformation[] = [];


If we can somehow pass this in as a reference, it might cheapen up the resource cost of this recursion.

d3r3kk · 2018-08-30T22:03:41Z

src/client/providers/symbolProvider.ts

+            args,
+            token
+        );
+        const symbols: SymbolInformation[] = [];


This might be the value you send into the recursive routine to avoid creating new objects.

I disagree, the API should return an array rather than us passing a state bag that needs to be populated.
Internally the function can use a single array that is passed by ref.

Returning items is better than passing a state bag, e.g if the cost is very expensive then we can always use generators (with yield), that works perfectly here. But if we passed a state bag there's no way to use yield.

d3r3kk · 2018-08-30T22:17:56Z

Approving the changes after a fun academic conversation with @ericsnowcurrently on the cost/benefit and the true costs of his solution vs. my proposal. We are only ever traversing a single file gathering these symbols. Unless that single file is wonderfully complicated in terrible ways, the perf of this solution is not going to be a problem.

👍

DonJayamanne

.

DonJayamanne · 2018-08-31T06:34:31Z

src/client/common/utils.ts

+ *  'x.y'   -> ['x', 'y']
+ *  'x'     -> ['', 'x']
+ *  'x.y.z' -> ['x.y', 'z']
+ *  ''      -> ['', '']


This is illogical. You can't have a parent if name is empty.
It should return undefined, instead of returning incorrect info. This leads to the assumption that there is a parent when there isn't.

I see your point in a general sense. However, in this case how is '' different from undefined? Using undefined for this seems like more work for everyone.

however that's poor API design!!

I'd like to discuss this more, perhaps in teams, etc. We're not communicating well about this over github. :)

...and I want to make sure we're on the same page with this sort of stuff.

DonJayamanne · 2018-08-31T06:37:18Z

src/client/providers/symbolProvider.ts

+ * A representation of the symbol data the language server provides.
+ */
+interface ILSSymbolTree {
+    name: string;


Please remember, use interfaces only when implementing them in classes. Else use types

Yep. This was an attempt to get type conversion to work right. I ended up doing it a different way but forgot to remove the interface. :)

DonJayamanne · 2018-08-31T06:38:12Z

src/client/providers/symbolProvider.ts

+    detail?: string;
+}
+interface IRange {
+    start: IPosition;


Surely there must be an existing interface we can use for this? Why can't we use what's in VSC?
I can't see where these are used internally!

Yep. :) This was also left over from trying to use type conversion.

DonJayamanne · 2018-08-31T06:39:09Z

src/client/unittests/codeLenses/testFiles.ts

+            if (!symbols) {
+                return [];
+            }
+            return symbols!


No need of !

This is your code, not mine. :) I'll fix it.

DonJayamanne · 2018-08-31T06:51:11Z

src/client/common/utils.ts

+ *  ''      -> ['', '']
+ */
+export function splitFullName(fullName: string): [string, string] {
+    if (fullName.length === 0) {


Please move this function into a separate file such a languageUtils.js, else this file here annoyed to store everything and it's become a very large bag of tricks!

I'll split up utils.ts in a separate PR.

DonJayamanne · 2018-08-31T07:05:00Z

src/client/providers/symbolProvider.ts

+            args,
+            token
+        );
+        const symbols: SymbolInformation[] = [];


I disagree, the API should return an array rather than us passing a state bag that needs to be populated.
Internally the function can use a single array that is passed by ref.

DonJayamanne · 2018-08-31T07:09:03Z

src/client/providers/symbolProvider.ts

+            args,
+            token
+        );
+        const symbols: SymbolInformation[] = [];


Returning items is better than passing a state bag, e.g if the cost is very expensive then we can always use generators (with yield), that works perfectly here. But if we passed a state bag there's no way to use yield.

DonJayamanne · 2018-08-31T07:12:50Z

src/test/providers/symbolProvider.unit.test.ts

+
+suite('Language Server Symbol Provider', () => {
+
+    function newLanguageClient(


Please rename to use a verb, such as createLanguageClient or get..

DonJayamanne · 2018-08-31T07:14:34Z

src/test/providers/symbolProvider.unit.test.ts

 });
+
+function newDoc(
+    uri?: Uri,


Please rename to createDoc or getDoc, or similar.

DonJayamanne · 2018-08-31T07:15:13Z

src/test/providers/symbolProvider.unit.test.ts

+// helpers
+
+function newSymbols(
+    uri: Uri,


Please rename to use a verb in the function name.
E.g. create, get, parse, etc.

I'm going to merge this with only a couple of Don's concerns still open. We can address those separately.

This is a follow-up to #2456. I've pulled many of the "util" modules out of src/client/common into a new sibling package to client: utils. This helps us separate the extension-specific code from the generic code we use for the extension.

DonJayamanne and others added 5 commits August 28, 2018 12:32

Use symbol provider to place code lenses in tests

0f5c4b0

Revert JediExtensionActivator -> ClassicExtensionActivator.

87965e6

Revert addition of ThrottledPythonSymbolProvider.

86e8fb6

PythonSymbolProvider -> JediSymbolProvider

cbb5c6a

SymbolProvider -> LanguageServerSymbolProvider

28ea0b3

brettcannon mentioned this pull request Aug 29, 2018

[WIP] Use symbol provider from new language server to place code lenses in tests #2197

Closed

7 tasks

ericsnowcurrently added 4 commits August 29, 2018 13:07

Add helpers.

e8fbc20

Add a test for LanguageServerSymbolProvider.

484a0a3

De-clutter the test.

ae64d4d

Add a NEWS entry.

9ca07cd

ericsnowcurrently changed the title ~~[WIP] Use symbol provider from new language server to place code lenses in tests.~~ Use symbol provider from new language server to place code lenses in tests. Aug 29, 2018

d3r3kk suggested changes Aug 30, 2018

View reviewed changes

d3r3kk approved these changes Aug 30, 2018

View reviewed changes

Flatten (and fix) the hierarchical symbol data returned from LS.

631b69a

ericsnowcurrently force-pushed the fix-1948-ls-test-code-lenses branch from cbad124 to 631b69a Compare August 30, 2018 22:28

Don't try to inject the filename.

4e1eb2a

DonJayamanne requested changes Aug 31, 2018

View reviewed changes

DonJayamanne previously requested changes Aug 31, 2018

View reviewed changes

ericsnowcurrently added 7 commits August 31, 2018 10:27

Explicitly convert fields in test data.

377fa13

Drop a superfluous compiler hint.

c573db9

Use DocumentSymbol instead of a local interface.

4f433ff

Make function names more verb-y.

8967834

splitFullName -> splitParent

6dada73

Support passing a number to parsePosition().

ecdf945

Add tests for the util functions.

32a6915

ericsnowcurrently merged commit 90d1e4a into microsoft:master Aug 31, 2018

ericsnowcurrently deleted the fix-1948-ls-test-code-lenses branch August 31, 2018 18:50

ericsnowcurrently mentioned this pull request Sep 5, 2018

Consolidate util code to src/util/*. #2489

Merged

6 tasks

This was referenced Sep 12, 2018

Symbol provider not available in unit tests when using new analysis engine #1706

Closed

Unit tests not working with the new analysis engine #1898

Closed

Consider reverting change used to throttle requests to symbol provider API #1869

Closed

lock bot locked as resolved and limited conversation to collaborators Jul 31, 2019


		suite('Language Server Symbol Provider', () => {

		function newLanguageClient(

Use symbol provider from new language server to place code lenses in tests. #2456

Use symbol provider from new language server to place code lenses in tests. #2456

Uh oh!

Conversation

ericsnowcurrently commented Aug 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ericsnowcurrently commented Aug 29, 2018

Uh oh!

ericsnowcurrently commented Aug 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ericsnowcurrently commented Aug 30, 2018

Uh oh!

ericsnowcurrently commented Aug 30, 2018

Uh oh!

d3r3kk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

d3r3kk commented Aug 30, 2018

Uh oh!

DonJayamanne left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

ericsnowcurrently commented Aug 28, 2018 •

edited

Loading

ericsnowcurrently commented Aug 29, 2018 •

edited

Loading

DonJayamanne left a comment •

edited

Loading