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

Go to Type Definition will show a prompt to update guru when used on something that doesn't have a type #2166

Closed
segevfiner opened this issue Dec 3, 2018 · 9 comments

Comments

@segevfiner
Copy link
Contributor

Steps to Reproduce:

  1. Use "Go to Type Definition" on something that is not a type. For example, use it on Println in the following:
package main

import "fmt"

func main() {
	fmt.Println("Hello, World!")
}
  1. You get the following:
    screen shot 2018-12-03 at 10 56 54

Using it on something that has a type works correctly.

VS Code 1.29.1
vscode-go 0.7.1-beta.3

@ramya-rao-a
Copy link
Contributor

ramya-rao-a commented Dec 4, 2018

Thanks for reporting @segevfiner

@hummerd, @segevfiner Our current method of identifying guru being stale seem to be catching a lot of false positives. Can any of you provide more information on the guru support here?

  • Is it that guru describe is the new feature or the fact that guru describe providing the position of the type in the new feature.
  • When was this feature of type position added to guru?

I'd rather us not show any prompt at all about updating guru, instead of showing it at wrong times.
I can add a note in the FAQ about updating guru to ensure this feature works.

@ramya-rao-a ramya-rao-a added the bug label Dec 4, 2018
@segevfiner
Copy link
Contributor Author

It's this commit: golang/tools@9c8bd46 (I think). It adds the required information to the existing describe command.

@hummerd
Copy link
Contributor

hummerd commented Dec 4, 2018

Type position is the new feature. I think extension can not separate this false cases from real ones - missing type position case (with old guru). Rest of guru's output is the same. So if we don't want this we should disable promtToUpdate at all. :(

@hummerd
Copy link
Contributor

hummerd commented Dec 4, 2018

Here is PR #2172 for logging instead of prompt

@ramya-rao-a
Copy link
Contributor

ramya-rao-a commented Dec 4, 2018

Thanks @segevfiner!

@hummerd, I didnt know you had implemented the feature in guru, nice job!

@ramya-rao-a
Copy link
Contributor

@hummerd Can we expect the typepos property to exist in newer guru all the time event if the value is null/undefined?

If yes, then we can check guruOutput.value && !guruOutput.value.hasOwnProperty('typepos') to decide if we want to update the guru?

@hummerd
Copy link
Contributor

hummerd commented Dec 4, 2018

Nop. We need patch guru for it. From guru's point of view it's ok. Why should it return values that are empty.

type DescribeValue struct {
	Type     string       `json:"type"`               // type of the expression
	Value    string       `json:"value,omitempty"`    // value of the expression, if constant
	ObjPos   string       `json:"objpos,omitempty"`   // location of the definition, if an Ident
	TypesPos []Definition `json:"typespos,omitempty"` // location of the named types, that type consist of
}

You can notice these omitempty tags on optional fields.

@hummerd
Copy link
Contributor

hummerd commented Dec 4, 2018

Thanks @segevfiner!

@hummerd, I didnt know you had implemented the feature in guru, nice job!

Thanks :) It was a multi-step move.

@ramya-rao-a
Copy link
Contributor

@segevfiner, @hummerd Can you confirm the fix using the latest beta version

@vscodebot vscodebot bot locked and limited conversation to collaborators Jan 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants