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

Use the newly added receiver output from godef to get the correct documentation #2223

Merged
merged 2 commits into from
Jan 9, 2019

Conversation

segevfiner
Copy link
Contributor

@segevfiner segevfiner commented Dec 31, 2018

Use the newly added receiver output from godef to get the correct documentation

See rogpeppe/godef#105

Part of #2107

⚠️ Depends on #2215 and the godef PR being merged

@segevfiner segevfiner changed the title godef receivers Use the newly added receiver output from godef to get the correct documentation Dec 31, 2018
Copy link
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.

If -r is not supported by the user's version of godef, then we should fall back to running godef without it. See https://github.com/Microsoft/vscode-go/blob/latest/src/goDeclaration.ts#L170 as a reference of how we do something similar when the -tags flag of gogetdoc is not supported.

Since this code path is very often visited (hover info and go to definition feature are most commonly used features), we can also look at having a variable that tracks if the -r flag is supported so that we can avoid seeing the error each time in the same VS Code session.

Regarding the prompt to update the tool, I do have it in all other cases of unsupported flags, but I wonder if we should skip it in this case.
In other cases, the missing flag was important to the feature in question.
Here, this flag is only important when the user is dealing with receivers.

Also, the unit tests are failing. Can you take a look at that?

@segevfiner
Copy link
Contributor Author

segevfiner commented Jan 8, 2019

Receivers are really common, I think it's best to have the user update IMHO.

A variable will cause us to not use -r once the user does update godef until he reloads VS Code.

@segevfiner
Copy link
Contributor Author

About https://github.com/Microsoft/vscode-go/blob/latest/src/goDeclaration.ts#L170... Does that even work?, It's a return inside the child_process.execFile callback, which is going to become the return value of the callback rather than the return value of the surrounding function.

@ramya-rao-a
Copy link
Contributor

ramya-rao-a commented Jan 8, 2019

About https://github.com/Microsoft/vscode-go/blob/latest/src/goDeclaration.ts#L170... Does that even work?,

I do recall testing this but now that I look at the code, I am doubtful. It should most likely be

if (stderr && stderr.startsWith('flag provided but not defined: -tags')) {
	p = null;
	return definitionLocation_gogetdoc(input, token, false).then(resolve, reject);
}

@ramya-rao-a
Copy link
Contributor

ramya-rao-a commented Jan 8, 2019

I take back my objection for keeping the prompt for updating the tool.

At this point, godef not only has the new -r flag but also has the module support. I was thinking of having some kind of update notification for the latter, but the current prompt will do the trick for both features.

But we do need the fallback to running without -r flag when it is not supported. promptForUpdatingTool has the logic to not prompt the update msg for the same tool twice in the same VS Code session if the user dismissed the previous update notification

@ramya-rao-a ramya-rao-a merged commit 8133ea3 into microsoft:master Jan 9, 2019
@segevfiner
Copy link
Contributor Author

segevfiner commented Jan 9, 2019

@ramya-rao-a Oh noes.... The PR for godef wasn't merged yet! Like this, it's just going to request people to update godef ad nauseum...

@ramya-rao-a
Copy link
Contributor

@segevfiner I trusted you!!!!

Lol, kidding. I should have checked if the upstream PR was merged or not.
Let me reach out to the project maintainers there.
For now, I'll just push a commit to not use the -r flag

@segevfiner
Copy link
Contributor Author

I did add a warning about this in the PR description, guess it didn't stand out enough enough. 🤷‍♂️

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