-
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
Move some ComputeRefactoringsAsync to common helpers 3/n #37196
Conversation
WIP:
Question:
ToDo:
|
src/EditorFeatures/CSharpTest/IntroduceUsingStatement/IntroduceUsingStatementTests.cs
Outdated
Show resolved
Hide resolved
src/Features/Core/Portable/CodeRefactorings/AbstractRefactoringHelpersService.cs
Outdated
Show resolved
Hide resolved
...e/Portable/IntroduceUsingStatement/AbstractIntroduceUsingStatementCodeRefactoringProvider.cs
Show resolved
Hide resolved
src/EditorFeatures/CSharpTest/RefactoringHelpers/RefactoringHelpersTests.cs
Show resolved
Hide resolved
...e/Portable/IntroduceUsingStatement/AbstractIntroduceUsingStatementCodeRefactoringProvider.cs
Show resolved
Hide resolved
src/Workspaces/CSharp/Portable/LanguageServices/CSharpSyntaxFactsService.cs
Show resolved
Hide resolved
src/Workspaces/CSharp/Portable/LanguageServices/CSharpSyntaxFactsService.cs
Show resolved
Hide resolved
src/Workspaces/CSharp/Portable/LanguageServices/CSharpSyntaxFactsService.cs
Outdated
Show resolved
Hide resolved
src/Workspaces/CSharp/Portable/LanguageServices/CSharpSyntaxFactsService.cs
Outdated
Show resolved
Hide resolved
src/Workspaces/Core/Portable/LanguageServices/SyntaxFactsService/AbstractSyntaxFactsService.cs
Outdated
Show resolved
Hide resolved
src/Workspaces/Core/Portable/LanguageServices/SyntaxFactsService/AbstractSyntaxFactsService.cs
Outdated
Show resolved
Hide resolved
src/Workspaces/Core/Portable/LanguageServices/SyntaxFactsService/AbstractSyntaxFactsService.cs
Outdated
Show resolved
Hide resolved
src/Workspaces/Core/Portable/LanguageServices/SyntaxFactsService/ISyntaxFactsService.cs
Outdated
Show resolved
Hide resolved
src/Workspaces/Core/Portable/LanguageServices/SyntaxFactsService/ISyntaxFactsService.cs
Outdated
Show resolved
Hide resolved
src/Workspaces/VisualBasic/Portable/LanguageServices/VisualBasicSyntaxFactsService.vb
Outdated
Show resolved
Hide resolved
src/Workspaces/Core/Portable/LanguageServices/SyntaxFactsService/AbstractSyntaxFactsService.cs
Outdated
Show resolved
Hide resolved
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.
Looks good. Minor cleanup/improvement suggestions.
Comment
Submit general feedback without explicit approval.
Ready to merge. |
@@ -71,7 +71,7 @@ private async Task HandleNonSelectionAsync(CodeRefactoringContext context) | |||
|
|||
// We offer the refactoring when the user is either on the header of a class/struct, | |||
// or if they're between any members of a class/struct and are on a blank line. | |||
if (!syntaxFacts.IsOnTypeHeader(root, textSpan.Start) && | |||
if (!syntaxFacts.IsOnTypeHeader(root, textSpan.Start, out _) && |
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 could just have 2 overloads with and without out param?
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.
saying it since there is 1 usage case where we want out param and all other cases it is not needed so, it feels reversed that all has to be changed for 1 usage case.
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.
or create extension method in same namespace.
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 idea was to be consistent with other IsOnXXXHeader methods. And since having the out param is essentially free I feel like the clutter of having overload isn't worth 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.
Generally, being consistent doesn't cost us anything (since we can discard the result) and makes everything simpler to understand.
/// - Selection is zero-width and touching a Token whose ancestor ends/starts precisely on current selection . | ||
/// - Selection is zero-width and touching a Token whose ancestor of type <typeparamref name="TSyntaxNode"/> ends/starts precisely on current selection. | ||
/// - Selection is zero-width and in whitespace that corresponds to a Token whose direct ancestor is of type of type <typeparamref name="TSyntaxNode"/>. | ||
/// - Selection is zero-width and in a header (defined by ISyntaxFacts helpers) of an node of type of type <typeparamref name="TSyntaxNode"/>. |
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.
so plan would be sharing it in all referactoring get node? even thought it is only 1 place used right now?
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 not sure I follow the question.
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 TrySelectedNode is used only by one place because cyrus prepared a helpful extension method helper on Context (and that one is used by a number (> 10) of refactorings already)
test failures? |
Continuation of work on #35525 and #35180