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

Do not show blank identifiers in file outline #1893

Merged

Conversation

ragurney
Copy link
Contributor

@ragurney ragurney commented Aug 30, 2018

Resolves #1889

@ragurney
Copy link
Contributor Author

Needs a test, having trouble getting the test.go file to work.

@ragurney ragurney force-pushed the gurney/DontIncludeBlankVarsInOutline branch from 629d51e to b353a3a Compare August 30, 2018 02:00
@ragurney ragurney changed the title [WIP] Do not show blank identifiers in file outline Do not show blank identifiers in file outline Aug 30, 2018
@ragurney ragurney force-pushed the gurney/DontIncludeBlankVarsInOutline branch from b353a3a to 7068517 Compare August 30, 2018 16:56
Copy link
Contributor

@ramya-rao-a ramya-rao-a left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

Since there is no immediate plans of having any other ignored symbols, lets skip using an array and compare the label directly with _

@ramya-rao-a
Copy link
Contributor

About the test, are you able to run the existing tests?

@ragurney
Copy link
Contributor Author

@ramya-rao-a I can run the existing tests, but four fail consistently each time:

  1) Go Extension Tests
base.js:240
       Test Definition Provider using gogetdoc:
     TypeError: Cannot read property 'uri' of null
      at vscode.workspace.openTextDocument.then.provider.provideDefinition.then.definitionInfo (/Users/rygurney/Code/personal/vscode-go/test/go.test.ts:92:33)
      at <anonymous>
  2) Go Extension Tests
base.js:240
       Test SignatureHelp Provider using gogetdoc:
     TypeError: Cannot read property 'signatures' of null
      at vscode.workspace.openTextDocument.then.testCases.map.provider.provideSignatureHelp.then.sigHelp (/Users/rygurney/Code/personal/vscode-go/test/go.test.ts:108:27)
      at <anonymous>
  3) Go Extension Tests
base.js:240
       Test Hover Provider using gogetdoc:
     TypeError: Cannot read property 'contents' of null
      at vscode.workspace.openTextDocument.then.testCases.map.provider.provideHover.then.res (/Users/rygurney/Code/personal/vscode-go/test/go.test.ts:141:80)
      at <anonymous>
  4) Go Extension Tests
base.js:240
       Gometalinter error checking:
      AssertionError [ERR_ASSERTION]: Failed to get linter results
      + expected - actual
      -false
      +true
      
      at util_1.getGoVersion.then.goCheck_1.check.then.diagnostics (/Users/rygurney/Code/personal/vscode-go/test/go.test.ts:404:12)
      at <anonymous>
      at process._tickCallback (internal/process/next_tick.js:188:7)

When trying to add a line such as var _ string = "foo", I get two new failures as well, which I haven't had time to look into yet and address. I'll try to get to the bottom of this tonight!

@ramya-rao-a
Copy link
Contributor

The gogetdoc related failures are because you don't have gogetdoc installed. Someone is working on updating the tests to not fail in such cases.

You can comment them and the gometalinter one while running your tests. Your changes are unrelated to them so you can safely ignore them.

When trying to add a line such as var _ string = "foo", I get two new failures as well,

That is mostly because there are other tests using the same file and now all the line numbers are changing. Try this

  • In the fixtures table, create a new folder for outline and add a go file there as your test file. Update all the outline related tests to use this file

@msftclas
Copy link

msftclas commented Aug 31, 2018

CLA assistant check
All CLA requirements met.

@ragurney ragurney force-pushed the gurney/DontIncludeBlankVarsInOutline branch from 81fbb26 to 91909a1 Compare August 31, 2018 02:44
@ramya-rao-a
Copy link
Contributor

@ragurney If you merge from master branch now, you will not get those failures

@ragurney
Copy link
Contributor Author

@ramya-rao-a sounds good, thanks! I think this PR is ready for another review.

@ramya-rao-a
Copy link
Contributor

Looks great, Thanks!

@ramya-rao-a ramya-rao-a merged commit 311208a into microsoft:master Aug 31, 2018
@ragurney ragurney deleted the gurney/DontIncludeBlankVarsInOutline branch August 31, 2018 20:19
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