Skip to content
This repository has been archived by the owner on Jul 15, 2023. It is now read-only.

Don't count signature position changes inside string literals #1738

Merged
merged 9 commits into from
Feb 20, 2019

Conversation

lggomez
Copy link
Contributor

@lggomez lggomez commented Jun 18, 2018

Fixes #1682

I noticed (just after pushing my branch) that there already is an util function here that uses regexes. I didn't run any benchmarks comparing performance (assuming both methods work the same)

@lggomez
Copy link
Contributor Author

lggomez commented Jun 18, 2018

@ramya-rao-a PTAL. I have to test it a bit more but it seems to fix the bug on the reported case of strings.Replace()

@lggomez
Copy link
Contributor Author

lggomez commented Jun 18, 2018

PS: this also fixes an out of bounds access here

@lggomez
Copy link
Contributor Author

lggomez commented Jun 27, 2018

Well, it covers all basic cases now, what follows is the composite strings like double quotes within back-ticks and viceversa:

screenshot_20180627_025125

@lggomez
Copy link
Contributor Author

lggomez commented Jul 2, 2018

@ramya-rao-a After several adjustments I think it is ready to try, it works for most of the basic scenarios I came up with. All ci jobs failed with a seemingly unrelated npm error, so you'll have to trigger another build

The following test fails locally but I see that it also fails on my local master branch, I'm not really sure why:

  2) Go Extension Tests
base.js:240
       Test Completion Snippets For Functions:
      AssertionError: 'funcAsVariable(${1:k})' == 'funcAsVariable(${1:k string})'
      + expected - actual
      -funcAsVariable(${1:k})
      +funcAsVariable(${1:k string})
      
      at vscode.workspace.openTextDocument.then.vscode.window.showTextDocument.then.provider.provideCompletionItemsInternal.then.items (DEV/GITHUB/vscode-go/test/go.test.ts:804:13)
      at <anonymous>

@lggomez
Copy link
Contributor Author

lggomez commented Jul 29, 2018

@ramya-rao-a PTAL

I wasn't available this last month due to work but now I'm returning to my regular schedule

Also, I couldn't find out the cause of this failure but it is a local issue only

@vladbarosan
Copy link
Contributor

@lggomez I hope dont mind I refactored the existing code and reused the method you mentioned

@lggomez
Copy link
Contributor Author

lggomez commented Feb 20, 2019

Not at all, this issue is long overdue so the fix will be really welcome into february's release 👍

Copy link
Member

@jhendrixMSFT jhendrixMSFT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good and works as expected :)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants