Skip to content

Gracefully fail when resolving unsupported symbolkeys in VB#53915

Merged
CyrusNajmabadi merged 1 commit intodotnet:mainfrom
CyrusNajmabadi:vbSymbolKey
Jun 7, 2021
Merged

Gracefully fail when resolving unsupported symbolkeys in VB#53915
CyrusNajmabadi merged 1 commit intodotnet:mainfrom
CyrusNajmabadi:vbSymbolKey

Conversation

@CyrusNajmabadi
Copy link
Contributor

No description provided.

@CyrusNajmabadi CyrusNajmabadi requested a review from a team as a code owner June 7, 2021 15:47
@ghost ghost added the Area-IDE label Jun 7, 2021
return default;
}

if (reader.Compilation.Language == LanguageNames.VisualBasic)
Copy link
Member

Choose a reason for hiding this comment

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

Didn't we have methods on syntax facts or somewhere that encodes this without hardcoding the language names directly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We do not. And this is not in an api that has any ability to interrogate that sort of thing. Effectively, we've shaped the api such that we're producing a string, and we 'resolve' that string againt a compilation and nothing else. We'd need to update the api to take a dependency on more ocmplex types (like HostServices) in order to utilize something like ISyntaxFacts which would be a large chagne i simply do not want to take.

Right now SymbolKey is only defined for C#/VB anyways (As are compilations) so this is both an effective and cheap way to deal with this to get the same result as any more complex option that we might want to take.

@CyrusNajmabadi CyrusNajmabadi merged commit 1bb6382 into dotnet:main Jun 7, 2021
@ghost ghost added this to the Next milestone Jun 7, 2021
@CyrusNajmabadi CyrusNajmabadi deleted the vbSymbolKey branch June 7, 2021 19:17
@RikkiGibson RikkiGibson modified the milestones: Next, 17.0.P2 Jun 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants