Add code refactoring to offer usings for fully qualified names#81484
Add code refactoring to offer usings for fully qualified names#81484CyrusNajmabadi merged 21 commits intomainfrom
Conversation
…efactoringProvider Co-authored-by: CyrusNajmabadi <4564579+CyrusNajmabadi@users.noreply.github.com>
Co-authored-by: CyrusNajmabadi <4564579+CyrusNajmabadi@users.noreply.github.com>
Co-authored-by: CyrusNajmabadi <4564579+CyrusNajmabadi@users.noreply.github.com>
…space Co-authored-by: CyrusNajmabadi <4564579+CyrusNajmabadi@users.noreply.github.com>
src/Features/Core/Portable/AddImport/AbstractAddImportCodeRefactoringProvider.cs
Show resolved
Hide resolved
src/Features/Core/Portable/AddImport/AbstractAddImportCodeRefactoringProvider.cs
Show resolved
Hide resolved
src/Features/Core/Portable/AddImport/AbstractAddImportCodeRefactoringProvider.cs
Show resolved
Hide resolved
src/Features/Core/Portable/AddImport/AbstractAddImportCodeRefactoringProvider.cs
Show resolved
Hide resolved
src/Features/Core/Portable/AddImport/AbstractAddImportCodeRefactoringProvider.cs
Show resolved
Hide resolved
src/Features/Core/Portable/AddImport/AbstractAddImportCodeRefactoringProvider.cs
Show resolved
Hide resolved
src/Features/Core/Portable/AddImport/AbstractAddImportCodeRefactoringProvider.cs
Show resolved
Hide resolved
…edback Co-authored-by: CyrusNajmabadi <4564579+CyrusNajmabadi@users.noreply.github.com>
Co-authored-by: CyrusNajmabadi <4564579+CyrusNajmabadi@users.noreply.github.com>
|
@copilot code and tests look good. Now implement the Visual Basic side. so you'll need the VB subclass of AbstractAddImportCodeRefactoringProvider, and you'll need equivalent VB tests for all scenarios. |
|
@davidwengier if this is also a feature/fix you want. |
…ring Co-authored-by: CyrusNajmabadi <4564579+CyrusNajmabadi@users.noreply.github.com>
Implemented VB support in commit 0a86c47:
|
| <value>Adds a project reference.</value> | ||
| </data> | ||
| <data name="Add_using_for_0" xml:space="preserve"> | ||
| <value>Add 'using {0};'</value> |
There was a problem hiding this comment.
This is entirely subjective so feel free to ignore, but I am surprised not to see "Simplify" name used here. To me, this code action is the same as the existing simplify action, it just doesn't need an existing using directive, and I would argue the fact that they are separate actions is an implementation detail.
There was a problem hiding this comment.
I contemplated a lot about how to fit this into the existing system and how to best present things. It's like a pseudo cross between add-import and simplify. Ultimately, it felt more like adding the import and the commensurate simplification fallout of that, vs simplifying a name by adding an import for it.
We could def play around with this.
There was a problem hiding this comment.
it just doesn't need an existing using directive, and I would argue the fact that they are separate actions is an implementation detail.
I hear you. I was thinking about that except that there is some pretty significant difference in the outcome between the two, which makes it less of an impl detail for me. Specifically 'simplify' is always 'safe' and 'local'. When you get it offered, it just makes the obvious change, right there, for what it is trying to do. Even fix-all, only simplifies in the exact way that would be expected.
This, on the other hand, is 'add using'. And, as such, even if you're adding the 'using' for one case, it may have broad impact on the entire file, needing to now qualify other references in teh file to keep them unambiguous. This takes it out of the realm of just 'simplify' for me.
| if (namespaceSymbol is null || namespaceSymbol.IsGlobalNamespace) | ||
| return; | ||
|
|
||
| // Check if there's already a using directive for this namespace |
There was a problem hiding this comment.
Is it possible/cheaper to check for diagnostics instead? If the other simplify action isn't being offered, then by definition there must not be a using in scope, and if it is, then we don't need to offer this one?
There was a problem hiding this comment.
I personally think that's more complex, and would require more intimate knowledge on the part of this refactoring on how teh code-analyzer works (and then calling into the analysis framework to either get all diags, or to fork an analyzer to just get those specific diags.
Easier to just actually check if hte using is already in scope :)
Summary
This PR implements a new code refactoring that offers to add using/Imports directives for fully qualified type names and simplify them.
Changes
AbstractAddImportCodeRefactoringProviderinsrc/Features/Core/Portable/AddImport/CSharpAddImportCodeRefactoringProviderinsrc/Features/CSharp/Portable/AddImport/VisualBasicAddImportCodeRefactoringProviderinsrc/Features/VisualBasic/Portable/AddImport/Behavior
Offers refactoring when cursor is on any part of a qualified type reference
Does not offer if using/Imports already exists for that namespace
Does not offer on nested types (T2 in T1.T2)
Does not offer inside using/Imports directives
First action simplifies only the invoked qualified name (plus any nested qualified names within it)
Second action finds and simplifies all types from the target namespace throughout the file
Fixes Offer to add usings for fully qualified names #51355
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.