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

WIP: Suggestion to expand func types to func snippets (#1553) #1560

Merged
merged 1 commit into from
May 13, 2018

Conversation

joncalhoun
Copy link
Contributor

@joncalhoun joncalhoun commented Mar 6, 2018

Given a type that defines a function, this change adds a code completion suggestion to expand the type into a closure. Eg:

type Demo func(int) string

Will expand into the the suggest with the name Demo and upon triggering it the following snippet will be used:

Demo(func(${1:arg1} int) {
  $2
})

Similarly, given the following type:

type Demo func(i int) string

The variable name that exists will be used (i in this example).

If accepted, this resolves #1553

@msftclas
Copy link

msftclas commented Mar 6, 2018

CLA assistant check
All CLA requirements met.

@joncalhoun
Copy link
Contributor Author

joncalhoun commented Mar 6, 2018

@ramya-rao-a I suspect this needs a test, but at first glance I'm not sure I understand the tests very well. I suspect I need to add something here - https://github.com/Microsoft/vscode-go/blob/0.6.77/test/go.test.ts#L698 - but that's about all the further I got. I'll probably dive into them more tomorrow, but I don't use typescript often so any help would probably speed this up.

The actual PR might be worth refactoring a bit, since a good chunk of the code is reused from the function building section, but it was different enough that I opted to duplicate the similar code. I'm open to changing it if desired.

Given a type that defines a function, added a code completion
suggestion to expand the type into a closure. Eg:

  type Demo func(int) string

Will expand into the the suggest with the name "Demo" and upon
triggering it the following snippet will be used:

  Demo(func(${1:arg1} int) {
    $2
  })
@joncalhoun
Copy link
Contributor Author

@ramya-rao-a I'd love to figure out what needs done to get this submitted.

I think it needs a test of some sort; can you point me to an area in the tests where you think this would be most easily tested?

After a test is added, is there anything else you feel will need altered in the PR?

@ramya-rao-a
Copy link
Contributor

There is just one test failing and that too only on tip, that's unrelated to your PR, I'll take a look at it soon.

Pointer to existing test for completion that you can use as reference: https://github.com/Microsoft/vscode-go/blob/0.6.78/test/go.test.ts#L747

You can add another test file to https://github.com/Microsoft/vscode-go/tree/0.6.78/test/fixtures/completions for your case and test it

@joncalhoun
Copy link
Contributor Author

Thank you!

@OneOfOne
Copy link
Contributor

OneOfOne commented May 1, 2018

ping

@joncalhoun
Copy link
Contributor Author

So I'm currently swamped and won't have time to finish this for at least a week. If you want to close this in the meantime its fine - I'll just reference it with a new PR when it is ready.

@ramya-rao-a
Copy link
Contributor

No worries, take your time.
If I get some time next week, then I'll add the test and merge the PR

@ramya-rao-a ramya-rao-a merged commit 9db6593 into microsoft:master May 13, 2018
@joncalhoun
Copy link
Contributor Author

👍 Thanks for the help on this. Psyched to see it make it into master finally 😄

@ramya-rao-a
Copy link
Contributor

Yup, I finally got around to adding some tests (See 8dd3f33)

I'll release an update early next week.

@ramya-rao-a
Copy link
Contributor

@joncalhoun I just realized that if the function has a return type, then the snippet doesnt reflect that.

@ramya-rao-a
Copy link
Contributor

Take the below code

package main

type Demo func(int) string

func main() {

	mycallback :=

	takeDemo(mycallback)
}

func takeDemo(callback Demo) {
	callback(1)
}

Next to mycallback := type D to get the completion. You will get

mycallback := Demo(func(arg1 int) {

	})

which wont compile as there is no return value in the func declaration

@joncalhoun
Copy link
Contributor Author

I think you are right looking the code - I see nothing there for return types. I must have forgot about that use case when writing it, and I haven't touched this code in a while.

Want me to take a stab at fixing it or are you going to? Regardless it might make sense to roll back the change since it can break things 😦

@ramya-rao-a
Copy link
Contributor

Fixed with 8bb08f6

There is no need to roll back.
A very small percentage of users have the setting to expand snippets and I doubt among them everyone will encounter this issue. Even if they do, the fix is easy, they just need to add the return type manually.

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.

Feature Request: Expand func types to func snippets
4 participants