Skip to content

Conversation

@nosami
Copy link
Contributor

@nosami nosami commented Jan 15, 2018

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.
/// </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
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

Copy link
Contributor

@KevinRansom KevinRansom left a comment

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
Copy link
Contributor Author

nosami commented Jan 16, 2018

@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
Copy link
Contributor

dsyme commented Jan 16, 2018

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
Copy link
Contributor Author

nosami commented Jan 16, 2018

OK, done.

@dsyme
Copy link
Contributor

dsyme commented Jan 17, 2018

@dotnet-bot Test Ubuntu14.04 Release_fcs Build please

Copy link
Member

@auduchinok auduchinok left a 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.

@KevinRansom
Copy link
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
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