Skip to content

Conversation

@dawedawe
Copy link
Contributor

@dawedawe dawedawe commented Mar 16, 2023

Fixes #14906

Works and tests now for

  • let Bindings
  • ActivePatterns
  • DU types
  • DU cases
  • Record types
  • Record fields
  • class types
  • class members
  • modules

Copy link
Contributor

@nojaf nojaf 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!

@dawedawe dawedawe marked this pull request as ready for review March 17, 2023 19:52
@dawedawe dawedawe requested a review from a team as a code owner March 17, 2023 19:52
@T-Gro
Copy link
Member

T-Gro commented Mar 20, 2023

Excellent finding!
It was working for (and only for?) function definitions before, right?

@dawedawe
Copy link
Contributor Author

Excellent finding! It was working for (and only for?) function definitions before, right?

It was also working for simple let bindings.
If you had something like:

/// Some Sig Doc on myC
val myC: int

in your .fsi file, that also worked.

Copy link
Contributor

@psfinaki psfinaki left a comment

Choose a reason for hiding this comment

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

Awesome job, @dawedawe, thanks a lot - looking forward to leverage this on the daily basis!

@psfinaki
Copy link
Contributor

Also, for reference, can you provide some screenshots of examples where this wasn't working before?

I tried some of the test cases but they already seem to work for me:
image

@dawedawe
Copy link
Contributor Author

Yes, Visual Studio seems to do something extra. So there are more cases in which VS shows the sig comments.
(I can't really verify as it's closed source. Or is there a way?)

ActivePatterns, Partial Active Patterns and class types for example don't work currently in VS, too.
If the tests in this PR are correct, they should afterwards.
I have currently some trouble with my local Rider build, so I can't readily provide screenshots, sorry.

@psfinaki
Copy link
Contributor

@dawedawe No worries, this is useful at least from the perspective of testing all this stuff. If you want to do some extra checks, feel free to, if not - that's also fine, we can merge this as-is I think.

@vzarytovskii
Copy link
Member

Yes, Visual Studio seems to do something extra. So there are more cases in which VS shows the sig comments.
(I can't really verify as it's closed source. Or is there a way?)

ActivePatterns, Partial Active Patterns and class types for example don't work currently in VS, too.
If the tests in this PR are correct, they should afterwards.
I have currently some trouble with my local Rider build, so I can't readily provide screenshots, sorry.

F# part of VS is open, you can check in the vsintegration directory in the repo
And more how to test it in the DEVGUIDE.md

@dawedawe
Copy link
Contributor Author

dawedawe commented Mar 20, 2023

Ok, managed to get some screens:
DiscUnion before:
discunion_before

DiscUnion after:
discunion_after

Module before:
module_before

Module after:
module_after

Type before:
type_before

Type after:
type_after

Type field before:
type_before

Type field after:
type_field_after

Mmh, the Active Patterns work in the tests, but not in my Rider instance.
Not sure if I did something wrong in the custom built or there's really an issue with them.
Will dig deeper tomorrow.

@dawedawe
Copy link
Contributor Author

F# part of VS is open, you can check in the vsintegration directory in the repo And more how to test it in the DEVGUIDE.md

Uh, thanks for the pointer. That's great. I really should have seen this earlier 😞

@dawedawe
Copy link
Contributor Author

dawedawe commented Mar 21, 2023

Ok, Active Patterns and Partial Active Patterns also work as expected. I lacked crucial mouse hovering skills yesterday 🤣

Active Pattern before:
ap_before

Active Pattern after:
ap_after

Partial Active Pattern before:
partial_ap_before

Partial Active Pattern after:
partial_ap_after

@auduchinok
Copy link
Member

@dawedawe The active pattern case is interesting: I expect the documentation to be available on the individual cases while its provided on the whole pattern, and it's not.

The last screenshots show that R# tooltip somehow got in instead of our F# ones, and it's not expected. We should fix it.

@nojaf
Copy link
Contributor

nojaf commented Mar 21, 2023

The last screenshots show that R# tooltip somehow got in instead of our F# ones, and it's not expected. We should fix it.

That feels related to the range of an active pattern not including the ( ).

@dawedawe
Copy link
Contributor Author

@auduchinok @nojaf
It seems to be range related, yes.
For the Partial Active Pattern Rider shows the tooltip with the signature doc comment when I hover over ( or _|)
For the Active Pattern Rider shows the tooltip with the signature doc comment when I hover over ( or )

@psfinaki
Copy link
Contributor

Thanks for all the screenshots :) As for the range fix, I guess it can be done separately as well - or here, I'm fine with both as long as it's tested.

@dawedawe
Copy link
Contributor Author

I'd prefer a separate one for that.

@psfinaki
Copy link
Contributor

Alright! Thanks, merging.

@psfinaki psfinaki merged commit e353fd0 into dotnet:main Mar 21, 2023
@auduchinok
Copy link
Member

auduchinok commented Mar 21, 2023

@auduchinok @nojaf
It seems to be range related, yes.

The problem is FCS doesn't return the documentation for the active pattern when you hovering a case, it's not related to parens/ranges. It might be a Rider-specific issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

Show Signature XML documentation in implementation file if absent

6 participants