Ensure method parameter tooltip/popup never occurs within strings or comments #2072

d3r3kk · 2018-06-29T07:43:31Z

Fixes #2057

This pull request:

Has a title summarizes what is changing
Includes a news entry file (remember to thank yourself!)
Has unit tests & code coverage is not adversely affected (within reason)
Works on all actively maintained versions of Python (e.g. Python 2.7 & the latest Python 3 release)
Works on Windows 10, macOS, and Linux (e.g. considered file system case-sensitivity)

- issue microsoft#2057 - struggling with test framework & signature provider!

- incorporate Don's vscMock for SignatureHelp - completely revert changes I'd made to completionSource tests - create new 'pythonSignatureProvider' test suite

d3r3kk · 2018-06-29T07:46:53Z

src/client/providers/completionSource.ts

-        if (position.character <= 0) {
-            return undefined;
-        }
-        const filename = document.fileName;


Not entirely clear why this block was here? Perhaps I'm missing something...

This code would skip processing for any line that begins with "[whitespace]//" which I believe is invalid for Python.

Please let me know if I should put this back (maybe embedded C-code within a .py file?).

It is possible

x = \ " //"

Yeah, that's true. However, the test here is for a line like this:
// some important thing in a comment...
or this:
// some comment...
...which isn't what Python would hold, I am certain.
(and the quotation marks in your example would fail the test I removed)

// some important thing in a comment...

This is NOT a comment.

d3r3kk · 2018-06-29T07:47:37Z

src/test/vscode-mock.ts

@@ -63,6 +63,7 @@ mockedVSCode.SnippetString = vscodeMocks.vscMockExtHostedTypes.SnippetString;
 mockedVSCode.EventEmitter = vscodeMocks.vscMock.EventEmitter;
 mockedVSCode.ConfigurationTarget = vscodeMocks.vscMockExtHostedTypes.ConfigurationTarget;
 mockedVSCode.StatusBarAlignment = vscodeMocks.vscMockExtHostedTypes.StatusBarAlignment;
+mockedVSCode.SignatureHelp = vscodeMocks.vscMockExtHostedTypes.SignatureHelp;


@DonJayamanne you rock so hard. I love that I was able to add this mock so easily.

codecov · 2018-06-29T09:05:36Z

Codecov Report

Merging #2072 into master will decrease coverage by 0.1%.
The diff coverage is 93.33%.

@@            Coverage Diff             @@
##           master    #2072      +/-   ##
==========================================
- Coverage   79.89%   79.79%   -0.11%     
==========================================
  Files         307      307              
  Lines       14064    14083      +19     
  Branches     2494     2499       +5     
==========================================
  Hits        11237    11237              
- Misses       2815     2834      +19     
  Partials       12       12

Flag	Coverage Δ
#MacOS	`73.99% <93.33%> (-0.06%)`	⬇️
#Windows	`74.08% <93.33%> (-0.1%)`	⬇️

Impacted Files	Coverage Δ
src/client/providers/completionSource.ts	`98.14% <100%> (+1.53%)`	⬆️
src/client/providers/signatureProvider.ts	`90% <100%> (+0.71%)`	⬆️
src/client/providers/providerUtilities.ts	`94.44% <87.5%> (-5.56%)`	⬇️
src/client/activation/analysis.ts	`25.86% <0%> (-2.35%)`	⬇️
src/client/debugger/PythonProcess.ts	`57.5% <0%> (-1.67%)`	⬇️
src/client/common/types.ts	`100% <0%> (ø)`	⬆️
src/client/common/configSettings.ts	`91.52% <0%> (+0.19%)`	⬆️
...rc/client/debugger/PythonProcessCallbackHandler.ts	`68.75% <0%> (+0.65%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d17f16f...c5c192e. Read the comment docs.

d3r3kk · 2018-06-29T09:08:45Z

src/client/providers/providerUtilities.ts

        // In case position is at the every end of the comment or unterminated string
-        index = tokens.getItemContaining(offset - 1);
+        index = tokens.getItemContaining(offset - 2);


I am having difficulty understanding this one. The given comment is misleading a bit - the test only looks for tokens that are of type TokenType.Comment, but in Python I don't think there is an end to a comment (like there is in Cx languages) - isn't it just # -> everything to EOL for comments in Python? Or is there a special <# ... #> style syntax?

# is a single line comment token.
We use """ or ''' for multilines
Let me if that answers your question.
If that's ok, then I can approve this PR.

~~Totally answers my question... so removing this block here was the right move I would think.~~

~~I'm actually writing a few more tests for the 'isInsideStringOrComment' at the moment, I'll see if those multi-line strings are handled properly or not.~~

...sorry - responded to the wrong thing. Blaming my tired brain.

- make it 1-based (was 0-based)

DonJayamanne

All ok,
Do remember, # is used only for single line comments.

DonJayamanne · 2018-06-29T15:23:02Z

src/client/providers/providerUtilities.ts

        // In case position is at the every end of the comment or unterminated string
-        index = tokens.getItemContaining(offset - 1);
+        index = tokens.getItemContaining(offset - 2);


# is a single line comment token.
We use """ or ''' for multilines
Let me if that answers your question.
If that's ok, then I can approve this PR.

DonJayamanne · 2018-06-29T16:20:25Z

src/client/providers/completionSource.ts

-        if (position.character <= 0) {
-            return undefined;
-        }
-        const filename = document.fileName;


It is possible

x = \ " //"

- determine whether or not cursors are in a string or comment

- previously expecting signatures within a string - this no longer occurs

d3r3kk · 2018-06-29T22:19:06Z

@DonJayamanne so based on the tests that were in place before I began this work, it appears that the signature was expected within a string. Was that the correct way for the extension to be? Or would the new behaviour (never showing function signatures within comments or strings, ever) be correct?

NOTE: I'm pefectly happy to abandon this work if that is the desired behaviour, I learned a ton about our test framework this week!

d3r3kk added 3 commits June 29, 2018 00:42

Discovered root cause & fixed issue with popups in comments

997c7b9

- issue microsoft#2057 - struggling with test framework & signature provider!

Add tests for signature provider

609ae44

- incorporate Don's vscMock for SignatureHelp - completely revert changes I'd made to completionSource tests - create new 'pythonSignatureProvider' test suite

News file

57bb883

d3r3kk added this to the Jul 2018 milestone Jun 29, 2018

d3r3kk requested a review from DonJayamanne June 29, 2018 07:43

d3r3kk commented Jun 29, 2018

View reviewed changes

d3r3kk added 2 commits June 29, 2018 02:03

Off-by-one problem in determination of tokens

5dd6e76

Commit the proper off-by-one fix...

677f7e3

d3r3kk commented Jun 29, 2018

View reviewed changes

Update index of cursor position for expected signature

d86ed09

- make it 1-based (was 0-based)

DonJayamanne approved these changes Jun 29, 2018

View reviewed changes

d3r3kk added 2 commits June 29, 2018 11:49

Add further tests

29afb84

- determine whether or not cursors are in a string or comment

Update unit tests for Singature (Jedi)

c5c192e

- previously expecting signatures within a string - this no longer occurs

d3r3kk merged commit c976b6c into microsoft:master Jul 2, 2018

d3r3kk deleted the tooltip-in-string branch July 2, 2018 21:54

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

Ensure method parameter tooltip/popup never occurs within strings or comments #2072

Ensure method parameter tooltip/popup never occurs within strings or comments #2072

d3r3kk commented Jun 29, 2018 •

edited

Loading

d3r3kk Jun 29, 2018

DonJayamanne Jun 29, 2018

d3r3kk Jun 29, 2018 •

edited

Loading

DonJayamanne Jun 29, 2018

d3r3kk Jun 29, 2018

codecov bot commented Jun 29, 2018 •

edited

Loading

d3r3kk Jun 29, 2018

DonJayamanne Jun 29, 2018

d3r3kk Jun 29, 2018 •

edited

Loading

DonJayamanne left a comment

DonJayamanne Jun 29, 2018

DonJayamanne Jun 29, 2018

d3r3kk commented Jun 29, 2018

Ensure method parameter tooltip/popup never occurs within strings or comments #2072

Ensure method parameter tooltip/popup never occurs within strings or comments #2072

Conversation

d3r3kk commented Jun 29, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

d3r3kk Jun 29, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jun 29, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

d3r3kk Jun 29, 2018 • edited Loading

Choose a reason for hiding this comment

DonJayamanne left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

d3r3kk commented Jun 29, 2018

d3r3kk commented Jun 29, 2018 •

edited

Loading

d3r3kk Jun 29, 2018 •

edited

Loading

codecov bot commented Jun 29, 2018 •

edited

Loading

d3r3kk Jun 29, 2018 •

edited

Loading