Fetch tooltip details on-demand for auto-completions #368

MikhailArkhipov · 2017-12-07T21:33:23Z

Removed fetching documentation along with completion
Added per-item documentation resolution
Moved completion and hover code to completion source and hover source so completion can use tooltip data
Cleaned up affected files

Closes #152

brettcannon · 2017-12-07T22:01:53Z

pythonFiles/completion.py

-            else:
-                _completion['description'] = self._generate_signature(
-                    signature)
+            # if self.show_doc_strings:


Any reason to keep this (and the other commented-out code) around?

Just in case we need it (or for reference purposes - i.e. how to do the doc fetch). But I am fine removing it, up to you folks.

I say strip it; we have a VCS for a reason. 😉

jeffwidman · 2017-12-07T22:31:25Z

Fix #152

FYI @MikhailArkhipov if you put this in your PR comment next time rather than the title, then github will auto-close the issue when the PR is merged.

…nto 152

…into 152

DonJayamanne · 2017-12-08T18:08:54Z

@MikhailArkhipov
Finally, please remove the following files (they are no longer used as a result of the code changes):

src/client/jedi/parsers/CompletionParser.ts
extractSignatureAndDocumentation function in src/client/providers/jediHelpers.ts

DonJayamanne · 2017-12-08T17:29:47Z

src/client/providers/completionSource.ts

+    public async getDocumentation(completionItem: vscode.CompletionItem, token: vscode.CancellationToken): Promise<string> {
+        const documentPosition = DocumentPosition.fromObject(completionItem);
+        if (documentPosition === undefined) {
+            Promise.resolve<string>('');


Do you mean to return something? This is a no-op statement.
I think you intend to return something,
Since you're in an async method, you can return the raw value (just as you would in C# 7)

return '';

DonJayamanne · 2017-12-08T17:31:28Z

src/client/providers/completionSource.ts

+
+    public static fromObject(item: object): DocumentPosition {
+        // tslint:disable-next-line:no-any
+        return (item as any).documentPosition as DocumentPosition;


Can we change the name of the property to _documentPosition or _pyDocumentPosition, so as to ensure this will not conflict with any future API vscode adds to the TextDocument object.
Cuz using any we'll never get any compilation errors (hence the dangers of using any).

DonJayamanne · 2017-12-08T17:34:30Z

src/client/providers/completionSource.ts

+        if (lineText.match(/^\s*\/\//)) {
+            return undefined;
+        }
+        // Suppress completion inside string and comments


Please end sentences with periods (.)

DonJayamanne · 2017-12-08T17:36:29Z

src/client/providers/completionSource.ts

+            (item.kind === vscode.SymbolKind.Function || item.kind === vscode.SymbolKind.Method)) {
+            completionItem.insertText = new vscode.SnippetString(item.text).appendText('(').appendTabstop().appendText(')');
+        }
+        // ensure the built in members are at the bottom


Please end sentence with period (.)

DonJayamanne · 2017-12-08T17:39:16Z

src/client/providers/jediHelpers.ts

@@ -1,8 +1,12 @@
-import * as proxy from './jediProxy';
+// Copyright (c) Microsoft Corporation. All rights reserved.


The license headers are to be added only for new files, not existing files. (please check with @brettcannon for further clarification).

DonJayamanne · 2017-12-08T17:58:42Z

src/client/providers/completionProvider.ts

-        const index = tokens.getItemContaining(document.offsetAt(position));
-        return index >= 0 && (tokens[index].TokenType === TokenType.String || tokens[index].TokenType === TokenType.Comment);
+    public async resolveCompletionItem(item: vscode.CompletionItem, token: vscode.CancellationToken): Promise<vscode.CompletionItem> {
+        item.documentation = await this.completionSource.getDocumentation(item, token);


We should probably check if item.documentation is empty or not. If not empty, then try to get the documentation. Else we get the documentation again.

Previous code used to initialize both the documentation and detail properties. I believe one is the signature of the method (display on the right of the method) and the other is the full documentation (displayed when you click on the i information icon).

I just tested the impact of the change, we loose the signature.
Guess we have two issues that need to be addressed:

Create unit tests to ensure documentation and details properties are validated (something I failed to add). Will be useful for future usage as well, if we use a language server or the like.

Ensure the above resolveCompletionItem populates both documentation and detail

I believe VSC is not calling resolve for items with documentation, but I'' double check.

Oh yes, that's true.

OK, I got the signature, good catch. But - choices, choices. Details is a plain string which is displayed on top of the documentation. But in a tooltip it is markdown/colorized. So, do we want TS way or display info with colors? I kinda like colored version...

I like the last item (colored version).

DonJayamanne · 2017-12-08T17:59:06Z

src/client/providers/hoverProvider.ts

@@ -1,107 +1,23 @@
+// Copyright (c) Microsoft Corporation. All rights reserved.
+// Licensed under the MIT License.


Please remove the license header. They should only be added for new files.

DonJayamanne · 2017-12-08T17:59:41Z

src/client/providers/completionProvider.ts

@@ -1,76 +1,35 @@
+// Copyright (c) Microsoft Corporation. All rights reserved.
+// Licensed under the MIT License.


Please remove the license header, they should be added for new files.

Only new files?

Yes, apparently that's what CELA have informed Brett.

DonJayamanne · 2017-12-09T01:18:00Z

src/client/providers/completionProvider.ts

+                // Only filling documentation since tooltip already contains
+                // all the necessary information and is colorized via markdown.
+                item.documentation = itemInfos[0].tooltip;
+                item.detail = '';


@MikhailArkhipov
I think we'll need the details, lets talk on Monday.

Without detail, you loose the signature displayed for every item (see below).
This is something that was requested some time ago in the extension (in some issue).

Not sure which one is which (documentation vs detail).

We'll chat Monday

DonJayamanne · 2017-12-09T01:36:28Z

@MikhailArkhipov The travis test is failing in one place, thats been fixed in a PR of mine. execObservable should output print statements (removed that test as I've changed the way results are streamed out, and the last item sent in some cases is just the crlf).

…nto 152

* upstream/master: Fetch tooltip details on-demand for auto-completions (#368) Fix interpreter display (#391)

MikhailArkhipov added 25 commits December 1, 2017 10:18

Basic tokenizer

7675901

Fixed property names

eb42669

Tests, round I

2756974

Tests, round II

c2c1ced

merge master

a108c96

tokenizer test

14864a5

Remove temorary change

0ed51d6

Fix merge issue

51b544c

Merge conflict

3cd11e6

Merge conflict

82e0ad1

Completion test

9295c1a

Fix last line

06eb1a5

Fix javascript math

e9db8e0

Merge master

d12ca03

Make test await for results

d8ab041

Add license headers

db75cd0

Rename definitions to types

9ab2c47

Round I

d9c95d0

License headers

d587485

Merge branch 'master' of https://github.com/Microsoft/vscode-python

1da5e0a

Merge branch 'master' into 152

3ac66b6

Separate completion and doc fetching

94a5a7e

Test fixes

df5af0e

Merge master

af5c648

Undo temp change

fdb0e57

DonJayamanne self-requested a review December 7, 2017 21:48

brettcannon reviewed Dec 7, 2017

View reviewed changes

Mikhail Arkhipov added 2 commits December 7, 2017 14:32

Merge branch 'master' into 152

c67ac49

Merge branch 'master' of https://github.com/Microsoft/vscode-python i…

3f27b3b

…nto 152

Merge branch '152' of https://github.com/MikhailArkhipov/vscode-python …

7163a1e

…into 152

DonJayamanne mentioned this pull request Dec 8, 2017

Improve performance of automatic environment detection #372

Closed

DonJayamanne requested changes Dec 8, 2017

View reviewed changes

MikhailArkhipov added 2 commits December 8, 2017 12:12

CR feedback

2c8b179

Restore signature, make info colorized

3689667

DonJayamanne requested changes Dec 9, 2017

View reviewed changes

brettcannon changed the title ~~#152 takes too much time to load pandas intellisense~~ Fetch tooltip details on-demand for auto-completions Dec 11, 2017

MikhailArkhipov added 3 commits December 11, 2017 13:19

Provide details string

e193805

Merge branch 'master' of https://github.com/Microsoft/vscode-python i…

ed48871

…nto 152

Fix tests

92655ec

DonJayamanne approved these changes Dec 11, 2017

View reviewed changes

MikhailArkhipov merged commit 8d79301 into microsoft:master Dec 11, 2017

MikhailArkhipov deleted the 152 branch December 13, 2017 22:46

DonJayamanne added a commit that referenced this pull request Dec 14, 2017

Merge remote-tracking branch 'upstream/master'

2c19004

* upstream/master: Fetch tooltip details on-demand for auto-completions (#368) Fix interpreter display (#391)

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fetch tooltip details on-demand for auto-completions #368

Fetch tooltip details on-demand for auto-completions #368

MikhailArkhipov commented Dec 7, 2017 •

edited by brettcannon

Loading

brettcannon Dec 7, 2017

MikhailArkhipov Dec 7, 2017

brettcannon Dec 7, 2017

MikhailArkhipov Dec 8, 2017

jeffwidman commented Dec 7, 2017

DonJayamanne commented Dec 8, 2017

DonJayamanne Dec 8, 2017

DonJayamanne Dec 8, 2017

MikhailArkhipov Dec 8, 2017

DonJayamanne Dec 8, 2017

DonJayamanne Dec 8, 2017

DonJayamanne Dec 8, 2017

DonJayamanne Dec 8, 2017

DonJayamanne Dec 8, 2017

MikhailArkhipov Dec 8, 2017

DonJayamanne Dec 8, 2017

MikhailArkhipov Dec 8, 2017

DonJayamanne Dec 8, 2017

DonJayamanne Dec 8, 2017

DonJayamanne Dec 8, 2017

MikhailArkhipov Dec 8, 2017

DonJayamanne Dec 8, 2017

DonJayamanne Dec 9, 2017

DonJayamanne Dec 9, 2017

DonJayamanne Dec 9, 2017

MikhailArkhipov Dec 9, 2017

MikhailArkhipov Dec 9, 2017

DonJayamanne commented Dec 9, 2017

		@@ -1,8 +1,12 @@
		import * as proxy from './jediProxy';
		// Copyright (c) Microsoft Corporation. All rights reserved.

		@@ -1,107 +1,23 @@
		// Copyright (c) Microsoft Corporation. All rights reserved.
		// Licensed under the MIT License.

		@@ -1,76 +1,35 @@
		// Copyright (c) Microsoft Corporation. All rights reserved.
		// Licensed under the MIT License.

Fetch tooltip details on-demand for auto-completions #368

Fetch tooltip details on-demand for auto-completions #368

Conversation

MikhailArkhipov commented Dec 7, 2017 • edited by brettcannon Loading

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

jeffwidman commented Dec 7, 2017

DonJayamanne commented Dec 8, 2017

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

DonJayamanne commented Dec 9, 2017

MikhailArkhipov commented Dec 7, 2017 •

edited by brettcannon

Loading