#34, #110 - suppress Intellisense in strings and comments #339

MikhailArkhipov · 2017-12-04T01:28:16Z

Basic tokenizer based on RTVS (comments and strings), can be extended as needed
Supporting classes
Tests

DonJayamanne

Please ensure all files end with blank line.
Please install the EditorConfig extension for vscode, that'll help.

DonJayamanne

Please ensure all files end with blank line.
Please install the EditorConfig extension for vscode, that'll help.

DonJayamanne · 2017-12-04T01:56:43Z

Tough to review in the phone, however I failed to notice any test for this feature. I.e. disable intellisense when inside a string. I think we'll need that.

Over all 👍

MikhailArkhipov · 2017-12-04T15:03:08Z

Yes Will do

brettcannon · 2017-12-04T21:42:52Z

Do we have an issue(s) tracking this change?

MikhailArkhipov · 2017-12-05T01:41:26Z

#34, #110

DonJayamanne · 2017-12-04T01:49:54Z

src/client/language/definitions.ts

@@ -0,0 +1,84 @@
+'use strict';
+


Please rename file to types.ts (that'll be the standard going forward, documented in standards document)

Ensure types.ts only contain interfaces. I.e. please move TextRange out into septate file

Actually it is used as both interface and direct type (i.e. new TextRange). Types that are not normally used directly, are in separate files. So basically definitions is public surface.

DonJayamanne · 2017-12-04T02:00:42Z

src/client/language/characterStream.ts

@@ -0,0 +1,131 @@
+'use strict';
+


Please add Microsoft license header to all new files.
Also please remove use strict (not necessary)

Can't find the license header sample, where is the template

NVM, found it

The official link is https://docs.opensource.microsoft.com/releasing/copyright-headers.html

DonJayamanne · 2017-12-04T19:42:47Z

src/test/autocomplete/base.test.ts

+            assert(vscode.window.activeTextEditor, 'No active editor');
+            textEditor = editor;
+            for (let i = 0; i < positions.length; i += 1) {
+              vscode.commands.executeCommand<vscode.CompletionList>('vscode.executeCompletionItemProvider',


I don't think the tests will work. Theoretically this should fail. Since this is an async operation, we have to wait for this to complete.
In line 211, we're basically saying we're all done, when in fact the code in line 204 is still running, i.e. tests won't/shouldn't work.

You'll have to wait for executeCommand to complete as done in line 117.

You could build a list of promises and then use Promise.all() or other mechanism.. But we need to wait for the completion of each.

@MikhailArkhipov this needs to be done.

DonJayamanne · 2017-12-05T01:48:26Z

src/test/index.ts

@@ -8,6 +8,7 @@ testRunner.configure({
    ui: 'tdd',
    useColors: true,
    timeout: 25000,
-    retries: 3
+    retries: 3,
+    grep: "Language.TextRangeCollection"


Oops, please remove this line.

@MikhailArkhipov this needs to be done

brettcannon · 2017-12-05T23:08:49Z

And with that, @MikhailArkhipov is now the 19th most prolific contributor to the extension! 😄

* upstream/master: #34, #110 - suppress Intellisense in strings and comments (#339) Re-factor code python execution framework (#345)

MikhailArkhipov added 10 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

DonJayamanne reviewed Dec 4, 2017

View reviewed changes

DonJayamanne requested changes Dec 4, 2017

View reviewed changes

Mikhail Arkhipov added 3 commits December 4, 2017 11:23

Completion test

9295c1a

Fix last line

06eb1a5

Fix javascript math

e9db8e0

Merge master

d12ca03

DonJayamanne reviewed Dec 5, 2017

View reviewed changes

MikhailArkhipov added 3 commits December 5, 2017 10:07

Make test await for results

d8ab041

Add license headers

db75cd0

Rename definitions to types

9ab2c47

DonJayamanne approved these changes Dec 5, 2017

View reviewed changes

MikhailArkhipov added 2 commits December 5, 2017 12:02

License headers

d587485

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

1da5e0a

MikhailArkhipov merged commit 1150480 into microsoft:master Dec 5, 2017

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

Merge remote-tracking branch 'upstream/master'

b16d2f9

* upstream/master: #34, #110 - suppress Intellisense in strings and comments (#339) Re-factor code python execution framework (#345)

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

#34, #110 - suppress Intellisense in strings and comments #339

#34, #110 - suppress Intellisense in strings and comments #339

MikhailArkhipov commented Dec 4, 2017 •

edited by brettcannon

Loading

DonJayamanne left a comment

DonJayamanne left a comment

DonJayamanne commented Dec 4, 2017

MikhailArkhipov commented Dec 4, 2017

brettcannon commented Dec 4, 2017

MikhailArkhipov commented Dec 5, 2017

DonJayamanne Dec 4, 2017

DonJayamanne Dec 4, 2017

MikhailArkhipov Dec 5, 2017

DonJayamanne Dec 4, 2017

MikhailArkhipov Dec 5, 2017

MikhailArkhipov Dec 5, 2017

brettcannon Dec 5, 2017

DonJayamanne Dec 4, 2017

DonJayamanne Dec 5, 2017

DonJayamanne Dec 5, 2017

DonJayamanne Dec 5, 2017

brettcannon commented Dec 5, 2017

#34, #110 - suppress Intellisense in strings and comments #339

#34, #110 - suppress Intellisense in strings and comments #339

Conversation

MikhailArkhipov commented Dec 4, 2017 • edited by brettcannon Loading

DonJayamanne left a comment

Choose a reason for hiding this comment

DonJayamanne left a comment

Choose a reason for hiding this comment

DonJayamanne commented Dec 4, 2017

MikhailArkhipov commented Dec 4, 2017

brettcannon commented Dec 4, 2017

MikhailArkhipov commented Dec 5, 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

brettcannon commented Dec 5, 2017

MikhailArkhipov commented Dec 4, 2017 •

edited by brettcannon

Loading