First steps in autocomplete #5601

rchiodo · 2019-05-10T22:26:36Z

For #5342

Rewire everything to use monaco and provide suggestions and hover support. Just use the base language server for now (jedi or dotnetcore)

Pull request represents a single change (i.e. not fixing disparate/unrelated things in a single PR)
Title summarizes what is changing
Has a news entry file (remember to thank yourself!)
Appropriate comments and documentation strings in the code
Has sufficient logging.
Has telemetry for enhancements.
Unit tests & system/integration tests are added/updated
Test plan is updated as appropriate
package-lock.json has been regenerated by running npm install (if dependencies have changed)
The wiki is updated with any design decisions/details.

…te_investigate

IanMatthewHuff · 2019-05-10T23:06:56Z

 "@types/react-codemirror": "^1.0.2",

Can be removed #Resolved

Refers to: package.json:2227 in a4f81e6. [](commit_id = a4f81e6, deletion_comment = False)

rchiodo · 2019-05-10T23:07:07Z

src/client/activation/languageServer/languageServer.ts


 @injectable()
 export class LanguageServer implements ILanguageServer {
- private readonly startupCompleted: Deferred<void>;


@DonJayamme if you could take a look at this part, that'd be great. I think making this all a singleton makes sense. But not entirely sure.

I think the LanguageServer was effectively a singleton before (as it's only created by the LanguageServerManager)

In reply to: 283065695 [](ancestors = 283065695)

rchiodo · 2019-05-10T23:12:34Z

 "@types/react-codemirror": "^1.0.2",

Will do.

In reply to: 491453735 [](ancestors = 491453735)

Refers to: package.json:2227 in a4f81e6. [](commit_id = a4f81e6, deletion_comment = False)

IanMatthewHuff · 2019-05-10T23:22:02Z

src/client/datascience/codeCssGenerator.ts

+ }
+
+ // Force the colors to the defaults
+ return null;


More of a question here. Why null for functions like this? As far as I'd seen the general though was always using undefined versus null unless specifically required by an external API. Want to make sure that we are both coding with the right concept of null versus undefined. #WontFix

VSCode and Typescript compiler have a general undefined over null rule that they mostly stick to.

In reply to: 283067624 [](ancestors = 283067624)

I think it's because some of the apis I was using were returning null colors.

In reply to: 283067624 [](ancestors = 283067624)

Yeah not sure vscode returns undefined. I think they actually return null for a lot of stuff. At least the completion stuff did. I think 'null' means no value returned, as opposed to not sure if the function was called or not.

In reply to: 283067802 [](ancestors = 283067802,283067624)

Yeah this says the same thing:
https://basarat.gitbooks.io/typescript/docs/javascript/null-undefined.html

Null and Undefined
Free youtube video on the subject
JavaScript (and by extension TypeScript) has two bottom types : null and undefined. They are intended to mean different things:
Something hasn't been initialized : undefined.
Something is currently unavailable: null.

In reply to: 283067961 [](ancestors = 283067961,283067802,283067624)

If the APIs return it that's enough of a reason for me no need to change. Sounds like (in the second link) some VSCode folks want only undefined and some want to use it like the link you posted.
https://github.com/Microsoft/TypeScript/wiki/Coding-guidelines#null-and-undefined
microsoft/vscode#70020

In reply to: 283068106 [](ancestors = 283068106,283067961,283067802,283067624)

Here's another example:

node_modules\vscode-languageserver-protocol\lib\protocol.d.ts

returns '| null' on just about everything. There's no '| undefined' anywhere.

In reply to: 283068106 [](ancestors = 283068106,283067961,283067802,283067624)

Oh it's DOM apis that return null. Looks like that's the root of where it comes from.

In reply to: 283068385 [](ancestors = 283068385,283068106,283067961,283067802,283067624)

IanMatthewHuff · 2019-05-10T23:33:03Z

src/client/datascience/codeCssGenerator.ts

+ // https://github.com/Microsoft/vscode/blob/master/src/vs/editor/standalone/common/themes.ts#L13
+ private generateMonacoThemeObject(args: IApplyThemeArgs) : monacoEditor.editor.IStandaloneThemeData {
+ const result: monacoEditor.editor.IStandaloneThemeData = {
+ base: args.isDark ? 'vs-dark' : 'vs',


Isn't it vs-light for the light theme? #Resolved

Not in monaco. See this as an example:
https://microsoft.github.io/monaco-editor/playground.html#customizing-the-appearence-exposed-colors

In reply to: 283069004 [](ancestors = 283069004)

Or this:
https://microsoft.github.io/monaco-editor/api/modules/monaco.editor.html#builtintheme

In reply to: 283069188 [](ancestors = 283069188,283069004)

IanMatthewHuff · 2019-05-10T23:42:13Z

src/client/datascience/codeCssGenerator.ts

+ if (args.baseColors) {
+ const keys = Object.keys(args.baseColors);
+ keys.forEach(k => {
+ const color = args.baseColors && args.baseColors[k] ? args.baseColors[k] : '#000000';


args.baseColors [](start = 30, length = 15)

Already checked at if #WontFix

Linter doesn't think so ;(

In reply to: 283070043 [](ancestors = 283070043)

IanMatthewHuff · 2019-05-10T23:52:48Z

src/client/datascience/codeCssGenerator.ts

+
+ // This theme may include others. If so we need to combine the two together
+ const include = theme ? theme.include : undefined;
+ if (include && include !== null) {


null should already be falsy. #Resolved

Will fix.

In reply to: 283071173 [](ancestors = 283071173)

IanMatthewHuff · 2019-05-10T23:58:13Z

src/client/datascience/themeFinder.ts

+ // Should be somewhere under currentPath/resources/app/extensions inside of a json file
+ let extensionsPath = path.join(currentPath, 'resources', 'app', 'extensions');
+ if (!(await fs.pathExists(extensionsPath))) {
+ // Might be on mac or linux. try a different path


Don't we have an IsWindows that we use other places? #WontFix

Yes, we do. But it still needs to check the first path I think.

In reply to: 283071695 [](ancestors = 283071695)

I guess I'd feel more comfortable checking both.

In reply to: 283071819 [](ancestors = 283071819,283071695)

Fine by me.

In reply to: 283072200 [](ancestors = 283072200,283071819,283071695)

IanMatthewHuff · 2019-05-14T17:43:53Z

src/client/datascience/history/intellisense/conversion.ts

+ incomplete: list.isIncomplete
+ };
+ } else {
+ const array = result as vscodeLanguageClient.CompletionItem[];


If the result is a vscode.CompletionItem[] or vscode.CompletionList don't they both get cast as the array here? Or do they have the same property isIncomplete for the completion item and the completion list as the vscodeLanguageClient items? If so a comment would be nice to specify that. #Resolved

Yes vscode.CompletionList and vcodeLanguageClient.CompletionList both have the isIncomplete property. This else should only come through here if the result is one of the two array types.

I'll add a comment to that effect.

In reply to: 283921359 [](ancestors = 283921359)

Actually this is a good catch since one of those types isIncomplete is an optional parameter. It should be checking for 'items' instead.

In reply to: 283922093 [](ancestors = 283922093,283921359)

rchiodo · 2019-05-14T17:46:21Z

"information_for_contributors": [

This is the history for this file in vscode:
https://github.com/microsoft/vscode/commits/master/extensions/python/syntaxes/MagicPython.tmLanguage.json #Resolved

Refers to: resources/MagicPython.tmLanguage.json:2 in cfefcf0. [](commit_id = cfefcf0, deletion_comment = False)

rchiodo · 2019-05-14T17:54:00Z

"information_for_contributors": [

Last actual change was in aug 2018

In reply to: 492339165 [](ancestors = 492339165)

Refers to: resources/MagicPython.tmLanguage.json:2 in cfefcf0. [](commit_id = cfefcf0, deletion_comment = False)

IanMatthewHuff · 2019-05-14T18:12:38Z

src/datascience-ui/history-react/code.tsx

+ private submitContent = () => {
+ let content = this.getContents();
+ if (content) {
+ // Remove empty lines off the end


Does it make sense to trim leading new lines as well? #WontFix

Maybe? I thought after was less obvious it was there so the user might not have intended it. On the front they did. But not sure why they'd want them. They just make the output cell bigger. Maybe somebody might do that on purpose though? Although I guess you could still achieve the same thing with a comment and bunch of space.

In reply to: 283932962 [](ancestors = 283932962)

IanMatthewHuff · 2019-05-14T18:21:24Z

src/datascience-ui/history-react/tokenizer.ts

+ {
+ comments: {
+ lineComment: '#',
+ blockComment: ['\'\'\'', '\'\'\'']


doc comments are ''' or """ #Resolved

These actually didn't seem to be used. I'll change it and see what happens. I think the tokenizer takes precedence.

In reply to: 283936732 [](ancestors = 283936732)

IanMatthewHuff

rchiodo added 30 commits April 18, 2019 10:08

Testing codemirror's autocomplete

4f44f84

Remember monacoEditor pieces

20f721d

New idea for using the language client

88ca7e1

Farther along the completion provider idea.

192db0d

New idea mapped together

00cb505

Merge remote-tracking branch 'origin/master' into rchiodo/auto_comple…

a98a817

…te_investigate

Closer. no more errors

f12e3f8

Merge remote-tracking branch 'origin/master' into rchiodo/auto_comple…

d49a60a

…te_investigate

Working completion

16df587

Sorting

fa93010

Reverse sort kind

0f738ae

Merge remote-tracking branch 'origin/master' into rchiodo/auto_comple…

00d0f3d

…te_investigate

Getting monaco to show context menu and completion

d8bacb0

Fix margin width

dc6cbde

Add comment about language service

e092f96

Start of completion provider

16a2913

Update to use monaco types instead

e23e7cc

New IHistoryListener idea

f2f6d9b

Fix document position

939dd5e

Theme support

54b9f6b

Colorization mostly working

53516a0

Merge remote-tracking branch 'origin/master' into rchiodo/auto_comple…

612e632

…te_investigate

Move editor to standalone panel at the bottom

c48c6fc

Fix startup of the language server

b31a54e

Merge from master

7003077

Input history working again

303d756

Fix problem with convertToOffset

df3fd6a

Write own react monaco editor to handle update problems

d75f48c

Add unit test for the completion provider

4a49ab6

Fix editing for intellisense

0d008f7

rchiodo self-assigned this May 10, 2019

Add news entry

a4f81e6

rchiodo commented May 10, 2019

View reviewed changes

Remove react-codemirror

79de88c

IanMatthewHuff reviewed May 10, 2019

View reviewed changes

Fix hygiene

01abc78

IanMatthewHuff reviewed May 10, 2019

View reviewed changes

Add variable to check remote

396dd9e

IanMatthewHuff reviewed May 10, 2019

View reviewed changes

Fix default rules to support comment/string beginnings

c1d28d1

IanMatthewHuff reviewed May 10, 2019

View reviewed changes

rchiodo added 5 commits May 10, 2019 16:59

Fix unnecessary isNull check

bd9c49b

Make sure to cleanup after waiting for the suggestion to show up

cfefcf0

Fix out of memory error.

c808487

Separate out the babel steps as not required for single workspace

a95001b

Fix color test

d4e84d6

IanMatthewHuff reviewed May 14, 2019

View reviewed changes

Review feedback

dc840df

IanMatthewHuff reviewed May 14, 2019

View reviewed changes

Update incorrect block comment

f246e57

IanMatthewHuff approved these changes May 14, 2019

View reviewed changes

rchiodo merged commit 2312c3e into master May 14, 2019

rchiodo deleted the rchiodo/auto_complete_investigate branch May 14, 2019 20:44

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

First steps in autocomplete #5601

First steps in autocomplete #5601

Uh oh!

Conversation

rchiodo commented May 10, 2019

IanMatthewHuff commented May 10, 2019 • edited by rchiodo Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rchiodo commented May 10, 2019

IanMatthewHuff May 10, 2019 • edited by rchiodo Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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

IanMatthewHuff May 10, 2019 • edited by rchiodo Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IanMatthewHuff May 10, 2019 • edited by rchiodo Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IanMatthewHuff May 10, 2019 • edited by rchiodo Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IanMatthewHuff May 10, 2019 • edited by rchiodo Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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

IanMatthewHuff May 14, 2019 • edited by rchiodo Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rchiodo commented May 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

rchiodo commented May 14, 2019

IanMatthewHuff May 14, 2019 • edited by rchiodo Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IanMatthewHuff May 14, 2019 • edited by rchiodo Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IanMatthewHuff left a comment

Choose a reason for hiding this comment

Labels

3 participants

IanMatthewHuff commented May 10, 2019 •

edited by rchiodo

Loading

IanMatthewHuff May 10, 2019 •

edited by rchiodo

Loading

IanMatthewHuff May 10, 2019 •

edited by rchiodo

Loading

IanMatthewHuff May 10, 2019 •

edited by rchiodo

Loading

IanMatthewHuff May 10, 2019 •

edited by rchiodo

Loading

IanMatthewHuff May 10, 2019 •

edited by rchiodo

Loading

IanMatthewHuff May 14, 2019 •

edited by rchiodo

Loading

rchiodo commented May 14, 2019 •

edited

Loading

IanMatthewHuff May 14, 2019 •

edited by rchiodo

Loading

IanMatthewHuff May 14, 2019 •

edited by rchiodo

Loading