-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Add records support in navigate to #48116
Add records support in navigate to #48116
Conversation
@@ -65,6 +65,7 @@ private static async Task<ProjectIndex> CreateIndexAsync(Project project, Cancel | |||
{ | |||
switch (info.Kind) | |||
{ | |||
// TODO: Not sure what is the correct action for records. |
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.
Should I rename classesThatMayDeriveFromSystemObject
to typesThatMayDeriveFromSystemObject
and treat DeclaredSymbolInfoKind.Record
the same as DeclaredSymbolInfoKind.Class
?
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.
don't call it typesThatMayDeriveFromSystemObject
as that would be everything. instead, call it classesOrRecordsThat...
src/Features/Core/Portable/Navigation/NavigableItemFactory.DeclaredSymbolNavigableItem.cs
Outdated
Show resolved
Hide resolved
}", async w => | ||
{ | ||
var item = (await _aggregator.GetItemsAsync("Goo")).Single(x => x.Kind != "Method"); | ||
VerifyNavigateToResultItem(item, "Goo", "[|Goo|]", PatternMatchKind.Exact, NavigateToItemKind.Record, Glyph.ClassInternal); |
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.
The NavigateToItemKind
used here doesn't have "Record" in it.
I think this one comes from a package reference "Microsoft.VisualStudio.Language.NavigateTo.Interfaces"
@CyrusNajmabadi What's the correct action here?
Is this package developed in this same repo or it's internal? Does it need to be updated first? Why we even need it if we have another NavigateToItemKind available?
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.
We woudl need to map our type to something in the existing enum.
Is this package developed in this same repo or it's internal?
It's part of VS, outside of Roslyn :)
Does it need to be updated first?
Nope. We can just map our concepts to theirs. We can decide later if they need to expose the concept as well.
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.
@CyrusNajmabadi I'm not sure what you meant by this.
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.
come to discord and we can discuss things there :)
Thanks for fixing this. Could you add an entry to the IDE test plan? I wasn't aware this was a separate scenario. |
@@ -229,6 +229,48 @@ private void ReportMatchResult(Project project, INavigateToSearchResult result) | |||
_callback.AddItem(navigateToItem); | |||
} | |||
|
|||
private static string GetKind(string kind) |
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.
@CyrusNajmabadi This is per our discussion in discord.
I hope I did it the correct way.
src/Features/Core/Portable/Navigation/NavigableItemFactory.DeclaredSymbolNavigableItem.cs
Outdated
Show resolved
Hide resolved
…laredSymbolNavigableItem.cs
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.
Thanks!
Before:
After: