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

Don`t process function signatures on comment lines #2496

Merged
merged 7 commits into from
May 7, 2019

Conversation

lggomez
Copy link
Contributor

@lggomez lggomez commented May 5, 2019

Fixes (at least part of) #2481


// Stop processing if the line is a comment
if (line.text.trim().indexOf('//') === 0) {
return null;
Copy link
Contributor

Choose a reason for hiding this comment

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

This will only catch the case where the entire line starts with the comment.
What about the below case?

x, err := regexp.Match("abc", data) // I have some comment here,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is the most basic case of course. Either way, I'm noticing the bug isn't completely dependent on comments presence so there is still something else in it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I decided to reuse the same function made for suggestions logic on this one

// if its current line, get the text until the position given, otherwise get the full line.
const [currentLine, characterPosition] = lineNr === position.line
? [line.text.substring(0, position.character), position.character]
: [line.text, line.text.length - 1];

for (let char = characterPosition - 1; char >= 0; char--) {
for (let char = characterPosition; char >= 0; char--) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sometime during the last year I introduced this regression with the belief this would prevent an issue in the future, and while even though the code path shouldn't have caused any issues I remained none the wiser 🤦‍♂️

Sorry for that. This should fix it

Copy link
Contributor

Choose a reason for hiding this comment

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

No problem @lggomez, thanks for the fix!

@ramya-rao-a ramya-rao-a merged commit 2642b53 into microsoft:master May 7, 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

Successfully merging this pull request may close these issues.

2 participants