Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Function tooltip shows documentation for its parameter #9117

Merged
merged 7 commits into from
May 9, 2020

Conversation

mcon
Copy link
Contributor

@mcon mcon commented May 3, 2020

Summary

I believe this change should fix this issue #7455, and have confirmed that it appears to do so when running an instance of ReSharper with this change built into it.
The method in question that's been changed LinesBefore, was behaving more like LinesAfter.

Testing

  • Manual testing with Rider
  • Added a new testcase which should cover this case, though I've not been able to run it due to a "did not resolve mscorelib" error when trying to run the FCS tests (I'm running on Windows, nothing out of the ordinary) - if someone could point me to how to go about running them, that would be great.

@dnfclas
Copy link

dnfclas commented May 3, 2020

CLA assistant check
All CLA requirements met.

Copy link
Contributor

@cartermp cartermp left a comment

Choose a reason for hiding this comment

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

Looks good, just a minor nit

@auduchinok
Copy link
Member

A failure here:

this.AssertQuickInfoContainsAtStartOfMarker (fileContent, "(*Marker1*)", "x1 param!")

@cartermp Is it OK that the test case seems to run (and fail) twice?

@mcon
Copy link
Contributor Author

mcon commented May 4, 2020

I'm getting a build error when trying to build the failing unit tests in question on master, is anyone able to suggest what the problem might be?

C:\Users\Matt\fsharp\tests\FSharp.Build.UnitTests\FSharp.Build.UnitTests.fsproj : error MSB4057: The target "_GetRestoreSettingsPerFramework" does not exist in the project.

@cartermp
Copy link
Contributor

cartermp commented May 4, 2020

@auduchinok Probably not, no 🙂

@cartermp
Copy link
Contributor

cartermp commented May 4, 2020

@mcon do you see that error when you build in VS?

@mcon
Copy link
Contributor Author

mcon commented May 4, 2020

When I use the command line build.cmd - I've got VS installed, but don't use use it as my IDE. Could be something to do with the fact I'd also built fcs in the same workspace - maybe new workspace time for me...

Managed to get things building, and tests running, looking at the failing test, it seems that the savedGrabPoints don't distinguish between parameters and function names, meaning fixing functions documentation breaks parameter documentation - I've found myself in pars.fsy - I'll have a go to see whether I'm able to fix this.

@mcon
Copy link
Contributor Author

mcon commented May 8, 2020

So, I've taken a look here @cartermp, and the secondary problem (which I've now resolved) was that the "grab point" for bindings was after the headBindingPattern, and so the XML lines which were being picked up included XML comments on the function arguments (which, for a tooltip on a function, I don't think is desirable). I've resolved this by putting a grab point at the start of the headBindingPattern, so that only the XML comment above the binding is picked up, and then one at the end of the pattern, so that any XML comments on arguments aren't picked up erroneously by other bindings down the file.

I wanted to have the XML comments on the arguments show up in tooltips too, but though I was able to add the grab points in the right place, I wasn't able to work out where to store the comments in such a way that they could be retrieved later, after parsing, by the tooltip. As a result I've not included this enhancement, and have just stuck to fixing the bug.

Worth noting that prior to this change, XML comments on the function arguments didn't show up in the tooltips - this behaviour isn't altered as part of this PR.

@cartermp
Copy link
Contributor

cartermp commented May 9, 2020

@mcon makes sense! Just fixing the bug here is definitely fine. Thanks!

@cartermp cartermp merged commit 45bbde6 into dotnet:master May 9, 2020
@mcon mcon deleted the correct-function-tooltip branch May 12, 2020 13:54
nosami pushed a commit to xamarin/visualfsharp that referenced this pull request Feb 23, 2021
* Function tooltip shows documentation for its parameter

* Missed array reversal

* Update test - debugging via CI...

* Update MultiProjectAnalysisTests.fs

* Removed supposedly redundent filtering to fix up tests

* Add additional grab point so that argument doc comments aren't included in binding comment.

* Review comments on tests.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants