Skip to content

Fix EventHookupCommandHandler for top-level statements#50726

Closed
Youssef1313 wants to merge 7 commits intodotnet:mainfrom
Youssef1313:patch-20
Closed

Fix EventHookupCommandHandler for top-level statements#50726
Youssef1313 wants to merge 7 commits intodotnet:mainfrom
Youssef1313:patch-20

Conversation

@Youssef1313
Copy link
Copy Markdown
Member

@Youssef1313 Youssef1313 commented Jan 23, 2021 •
edited
Loading

Fixes #49325

@Youssef1313 Youssef1313 requested a review from a team as a code owner January 23, 2021 12:26
@ghost ghost added the Area-IDE label Jan 23, 2021
@Youssef1313
Copy link
Copy Markdown
Member Author

Youssef1313 commented Jan 23, 2021

I'm going to close for now and will re-open in case I could fix it.

The issue #44425 seems very related.

When the following part is fixed to have the GlobalStatementSyntax when there is no ancestor of type TypeDeclarationSyntax, the code should go to the path in #44425.

roslyn/src/EditorFeatures/CSharp/EventHookup/EventHookupCommandHandler_TabKeyCommand.cs

Lines 232 to 234 in 3d60b75

var typeDecl = eventHookupExpression.GetAncestor<TypeDeclarationSyntax>();
var typeDeclWithMethodAdded = CodeGenerator.AddMethodDeclaration(typeDecl, generatedMethodSymbol, document.Project.Solution.Workspace, new CodeGenerationOptions(afterThisLocation: eventHookupExpression.GetLocation()));

However, my preference is to have any method generation process relying on the same code path. For example, ExtractMethod feature was fixed to support top-level statements. But this one doesn't depend on the same API. Unless there is a good reason for this separation, depending on a single API for this process is much more maintainable.

@Youssef1313 Youssef1313 deleted the patch-20 branch January 23, 2021 19:27
@Youssef1313 Youssef1313 restored the patch-20 branch January 24, 2021 10:30
@Youssef1313 Youssef1313 reopened this Jan 24, 2021
@Youssef1313 Youssef1313 mentioned this pull request Jan 24, 2021
Closed
@jinujoseph jinujoseph added the Community The pull request was submitted by a contributor who is not a Microsoft employee. label Jan 25, 2021
Base automatically changed from master to main March 3, 2021 23:53
@Youssef1313 Youssef1313 changed the title Test EvenHookupCommandHandler for top-level statements Fix EventHookupCommandHandler for top-level statements Jul 8, 2021
@Youssef1313
Copy link
Copy Markdown
Member Author

Youssef1313 commented Jul 8, 2021

@sharwell or @CyrusNajmabadi for small fix

@Youssef1313 Youssef1313 mentioned this pull request Dec 24, 2021
Merged
@Youssef1313
Copy link
Copy Markdown
Member Author

Youssef1313 commented Dec 24, 2021

Already fixed in #58475.

@Youssef1313 Youssef1313 deleted the patch-20 branch December 24, 2021 07:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@sharwell sharwell

Labels

Area-IDE Community The pull request was submitted by a contributor who is not a Microsoft employee.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Tab to insert new method for event handler crashed with top-level statements

3 participants

@Youssef1313 @sharwell @jinujoseph