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

Complete method receivers of types in namespace #168#1368

Merged
ramya-rao-a merged 3 commits intomicrosoft:masterfrom
grooveygr:methods
Dec 16, 2017
Merged

Complete method receivers of types in namespace #168#1368
ramya-rao-a merged 3 commits intomicrosoft:masterfrom
grooveygr:methods

Conversation

@grooveygr
Copy link
Copy Markdown
Contributor

No description provided.

@msftclas
Copy link
Copy Markdown

msftclas commented Nov 26, 2017

CLA assistant check
All CLA requirements met.

Copy link
Copy Markdown
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.

@grooveygr Nice work, I assumed I'd need list of all available types to provide this feature, but your way is so much more simpler and works perfectly :)

I have left 2 comments, let me know what you think of those.

src/goSuggest.ts Outdated
suggest.class === 'type' && !goBuiltinTypes.has(suggest.name)) {
let prefix = 'func (' + suggest.name[0].toLowerCase() + ' *' + suggest.name + ')';
let auxItem = new vscode.CompletionItem(suggest.name + ' method', vscode.CompletionItemKind.Snippet);
auxItem.label = suggest.name + ' (new method)';
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What do you think of 'func (*' + suggest.name + ') for label and Method snippet for detail?

Copy link
Copy Markdown
Contributor Author

@grooveygr grooveygr Dec 15, 2017

Choose a reason for hiding this comment

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

I experimented with your idea and found that the vscode engine does not show suggestions in some cases where the suggest.name is not a prefix of the label. More specifically, when completing an unexported type the suggestion is not shown.
For now I changed the label to be exactly suggest.name and kept your suggestion for the label.
Let me know what you think!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You can get around that by adding auxItem.filterText = suggest.name

By default the label is used for displaying the item as well as filtering the item based on what is typed. If filterText is set, then that will be used for filtering instead

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Nice! Learned something new.
Done.

src/goSuggest.ts Outdated
auxItem.label = suggest.name + ' (new method)';
auxItem.detail = prefix + '...';
auxItem.sortText = 'a';
let snippet = prefix + ' ${1:name}(${2:params}) ${3:retval} \{\n\t$0\n\}';
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Since params and retval can be empty, why not have them as simple tabstops instead of placeholders?

i.e ${1:methodName}(${2}) ${3} instead of ${1:name}(${2:params}) ${3:retvall}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done.

@ramya-rao-a ramya-rao-a merged commit c7fe79f into microsoft:master Dec 16, 2017
@ramya-rao-a
Copy link
Copy Markdown
Contributor

@grooveygr I had a question. Is it possible to have a method on a type from a different package?

@grooveygr
Copy link
Copy Markdown
Contributor Author

@ramya-rao-a according to the golang spec (https://golang.org/ref/spec#Method_declarations) adding methods to imported types is not supported.

The type denoted by T is called the receiver base type; it must not be a pointer or interface type and it must be defined in the same package as the method.

When you try, you're met with a compilation error, e.g. cannot define new methods on non-local type gzip.Header

@ramya-rao-a
Copy link
Copy Markdown
Contributor

Great, thanks for the clarification!

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