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

Triggering autocompletion in comments if the next item is an exported member #1005 - #1675

Merged
Ramya Rao (ramya-rao-a) merged 15 commits into
microsoft:masterfrom
shreyaskarnik:master
May 15, 2018
Merged

Triggering autocompletion in comments if the next item is an exported member #1005#1675
Ramya Rao (ramya-rao-a) merged 15 commits into
microsoft:masterfrom
shreyaskarnik:master

Conversation

@shreyaskarnik

Copy link
Copy Markdown
Contributor

This pull request adds functionality described #1005 by triggering autocompletion in comments if the next line is an exported member. This would be handy in adding documentation for exported members.

@msftclas

Microsoft Contribution License Agreements (msftclas) commented May 12, 2018

Copy link
Copy Markdown

CLA assistant check
All CLA requirements met.

Comment thread src/goSuggest.ts Outdated

if (lineText.match(/^\s*\/\//)) {
return resolve([]);
let nextLine = document.lineAt(position.line + 1).text;

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.

If cursor is in the last line of the file, this will result in error

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.

Thanks for the feedback. I will add logic to check for this and include an test case.

Comment thread src/goSuggest.ts Outdated
return resolve([]);
let nextLine = document.lineAt(position.line + 1).text;
let memberName = nextLine.replace(/\s+/g, ' ').trim().split(' ');
if (!nextLine.startsWith('\t')) {

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.

I am guessing your assumption here is that the file is always formatted, which might not be the case. Instead look for func, var and type declarations in the nextLine.

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.

Will take these considerations into account and modify code.

Comment thread src/goSuggest.ts Outdated
// check for exported members declared in block
memberName.shift();
}
if (!memberName[0].match(/^[A-Z]/)) {

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.

Say we found an exported member. In that case we should return a single item (the member name) in the suggestion list. Currently gocode gets called and it can return multiple results.

@shreyaskarnik Shreyas Karnik (shreyaskarnik) May 13, 2018

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.

In this case gocode should not be invoked right?

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.

Yes, we shouldn't invoke gocode. Instead we should return a single completion item with the member name right here

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.

Cool, will do.

@ramya-rao-a

Copy link
Copy Markdown
Contributor

Also, there have been conflicting changes in master. So first, merge from the master branch.

@shreyaskarnik

Shreyas Karnik (shreyaskarnik) commented May 13, 2018

Copy link
Copy Markdown
Contributor Author

Given the change and removal of if (lineText.match(/^\s*\/\//)) what would be the preference to proceed and check for condition where it is not a line comment and trigger the completion?

@ramya-rao-a Ramya Rao (ramya-rao-a) left a comment

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.

shreyu86 The changes are good. I just have a few more suggestions. Please take a look

Comment thread src/goSuggest.ts Outdated
// triggering completions in comments on exported members
if (position.line + 1 < document.lineCount) {
let nextLine = document.lineAt(position.line + 1).text.trim();
let memberType = nextLine.match(/(const|func|type|var)\s*(\w+)/);

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.

We need atleast one space between the keyword and member name, so \s* here should be s+

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.

Added the right regex.

GreetingStatus = 1
)

//

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.

Keep the cursor between the two / and manually trigger completions by pressing Ctrl+Space. We get the completion for SayHello, but we shouldn't

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.

Have added logic to handle this case.

Comment thread src/goSuggest.ts Outdated
const commentIndex = lineText.indexOf('//');
if (commentIndex >= 0 && position.character > commentIndex) {
return resolve([]);
if (!lineText.trim().startsWith('//')) {

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.

To avoid a lot of nesting, you can pull your code out and place it before line 55. You can start with

// triggering completions in comments on exported members
if (lineText.trim().startsWith('//') && (position.line + 1 < document.lineCount)) {
   let nextLine = .....
}

Comment thread src/goSuggest.ts Outdated
if (position.line + 1 < document.lineCount) {
let nextLine = document.lineAt(position.line + 1).text.trim();
let memberType = nextLine.match(/(const|func|type|var)\s*(\w+)/);
if (memberType && memberType.length === 3) {

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.

In the test file, at lines 11, 13, 18, 20 and 35 we get multiple completions from gocode and not from the code we specifically wrote for exported members.

This is because we don't exit with empty results when there is no match with the regex above.

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.

So in case of a block declaration, we should not trigger any completions and exit with empty results?

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.

Yes, unless you want to scan through the lines above and below the current line to look for the starting and ending of the block

@shreyaskarnik Shreyas Karnik (shreyaskarnik) May 14, 2018

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.

I am fine with skipping, wanted to clarify before I made any changes.

@shreyaskarnik

Copy link
Copy Markdown
Contributor Author

Thanks a lot! Ramya Rao (@ramya-rao-a)

@ramya-rao-a

Copy link
Copy Markdown
Contributor

Thanks for all the work shreyu86!
I've made 2 small changes, take a look if you want to :)

@ramya-rao-a
Ramya Rao (ramya-rao-a) merged commit 82481fe into microsoft:master May 15, 2018
@shreyaskarnik

Copy link
Copy Markdown
Contributor Author

Thanks a lot for the refactor, I really appreciate the swift feedback.

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