-
Notifications
You must be signed in to change notification settings - Fork 199
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 a code action to promote a using directive #11241
Conversation
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.
Great job! Nothing blocking. Some opinionated things and potential for less work in the provider.
return SpecializedTasks.EmptyImmutableArray<RazorVSInternalCodeAction>(); | ||
} | ||
|
||
var owner = syntaxTree.Root.FindNode(TextSpan.FromBounds(context.StartAbsoluteIndex, context.EndAbsoluteIndex)); |
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.
nit: We know context.HasSelection
is false so you only need StartAbsoluteIndex
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.
True, but there is no "TextSpan.FromPoint" where I can just pass in one argument, so I think I'll leave this, just in case we add selection support later and it hides a bug.
.../Microsoft.CodeAnalysis.Razor.Workspaces/CodeActions/Razor/PromoteUsingCodeActionProvider.cs
Outdated
Show resolved
Hide resolved
.../Microsoft.CodeAnalysis.Razor.Workspaces/CodeActions/Razor/PromoteUsingCodeActionProvider.cs
Outdated
Show resolved
Hide resolved
.../Microsoft.CodeAnalysis.Razor.Workspaces/CodeActions/Razor/PromoteUsingCodeActionProvider.cs
Outdated
Show resolved
Hide resolved
[Fact] | ||
public async Task PromoteUsingDirective_ExistingImports_WhitespaceLineAtEnd() | ||
{ | ||
await VerifyCodeActionAsync( |
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.
Is it worth adding a test that showcases moving from Imports/_ViewImports to one a folder up?
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.
This is really annoying to do with our current test infra :(
I'll have a go, but will time box it. I wouldn't mind cleaning up some of the code actions infra a bit more, as we add more, so can always come back to it.
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.
I'm giving up 🎉
Should be easier once the source generator is hooked up.
Fixes #6155
Me: I really like the cohosting code action tests, it must be really easy to add a new code action now!
Me: Oh really? Well, try it and find out, I dare you!!
Me: <this PR>
(Also me: That wasn't as much fun as I expected. I have thoughts.)