-
Notifications
You must be signed in to change notification settings - Fork 842
Get declaration list symbols #4204
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
Conversation
Now that we can include all entities for top level completions, the grouping function can incorrectly group types with the same display name as function overloads. Use the compiled name instead as this includes the namespace.
src/fsharp/service/service.fsi
Outdated
| /// </param> | ||
| /// <param name="userOpName">An optional string used for tracing compiler operations associated with this request.</param> | ||
| member GetDeclarationListSymbols : ParsedFileResultsOpt:FSharpParseFileResults option * line: int * lineText:string * partialName: PartialLongName * ?hasTextChangedSinceLastTypecheck: (obj * range -> bool) * ?userOpName: string -> Async<FSharpSymbolUse list list> | ||
| member GetDeclarationListSymbols : ParsedFileResultsOpt:FSharpParseFileResults option * line: int * lineText:string * partialName: PartialLongName * getAllSymbols: (unit -> AssemblySymbol list) * ?hasTextChangedSinceLastTypecheck: (obj * range -> bool) * ?userOpName: string -> Async<FSharpSymbolUse list list> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you make getAllSymbols optional please? The default should just be to return empty
@vasily-kirichenko or @auduchinok Can you review too please?
Thanks
KevinRansom
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
@KevinRansom That's no problem at all - I think that I am the only consumer of this API. I wanted to know if I should change Just seems a little odd if one API has |
Yes please, thank you |
Also renamed getAllSymbols to getAllEntities everywhere for consistency
|
OK, done. |
|
@dotnet-bot Test Ubuntu14.04 Release_fcs Build please |
auduchinok
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look good for me.
Making getAllEntities optional in GetDeclarationListInfo will add a lot of changes, it could be in a separate PR.
|
Thank you Kevin |
Mirror the change that was made to GetDeclarationListInfo API to add a function that gets all entities to GetDeclarationListSymbols.
Also changed the overload grouping mechanism to include the namespace. Previously this was grouping on DisplayName only, but this doesn't work when you return multiple types with the same DisplayName.