Skip to content

Get declaration list symbols#4204

Merged
KevinRansom merged 4 commits into
dotnet:masterfrom
nosami:GetDeclarationListSymbols
Jan 24, 2018
Merged

Get declaration list symbols#4204
KevinRansom merged 4 commits into
dotnet:masterfrom
nosami:GetDeclarationListSymbols

Conversation

@nosami

@nosami nosami commented Jan 15, 2018

Copy link
Copy Markdown
Contributor

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.

import-intellisense

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.
Comment thread 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>

Copy link
Copy Markdown
Contributor

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 KevinRansom left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@nosami
per @dsyme can we make getAllSymbols optional please.

@nosami

nosami commented Jan 16, 2018

Copy link
Copy Markdown
Contributor Author

@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 GetDeclarationListInfo to also be optional (currently, it isn't). GetDeclarationListInfo is used by Visual Studio, Rider and IonIDE already.

Just seems a little odd if one API has getAllSymbols optional and the other doesn't.

@dsyme

dsyme commented Jan 16, 2018

Copy link
Copy Markdown
Contributor

wanted to know if I should change GetDeclarationListInfo to also be optional (currently, it isn't). GetDeclarationListInfo is used by Visual Studio, Rider and IonIDE already.

Yes please, thank you

Also renamed getAllSymbols to getAllEntities everywhere for
consistency
@nosami

nosami commented Jan 16, 2018

Copy link
Copy Markdown
Contributor Author

OK, done.

@dsyme

dsyme commented Jan 17, 2018

Copy link
Copy Markdown
Contributor

@dotnet-bot Test Ubuntu14.04 Release_fcs Build please

@auduchinok auduchinok left a comment

Copy link
Copy Markdown
Member

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.

@KevinRansom

Copy link
Copy Markdown
Contributor

@nosami

Thank you

Kevin

@KevinRansom KevinRansom merged commit d70a450 into dotnet:master Jan 24, 2018
@nosami nosami deleted the GetDeclarationListSymbols branch January 24, 2018 09:55
T-Gro pushed a commit that referenced this pull request Jun 12, 2026
* Update GetDeclarationListSymbols to add getAllEntities

* Don't group types with the same display name as function overloads.

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.

* Get logical name from ExnCase

* Make getAllEntities optional

Also renamed getAllSymbols to getAllEntities everywhere for
consistency
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