Support abstract methods and records in complete statement command#48128
Support abstract methods and records in complete statement command#48128sharwell merged 12 commits intodotnet:mainfrom
Conversation
src/EditorFeatures/CSharpTest/CompleteStatement/CSharpCompleteStatementCommandHandlerTests.cs
Show resolved
Hide resolved
| [InlineData("abstract void M(object o = $$default(object))", "abstract void M(object o = default(object))")] | ||
| [InlineData("public record C(int X, $$int Y)", "public record C(int X, int Y)")] | ||
| [InlineData("public record C(int X, int$$ Y)", "public record C(int X, int Y)")] | ||
| [InlineData("public record C(int X, int Y$$)", "public record C(int X, int Y)")] |
There was a problem hiding this comment.
i see tests added for records, but nothing new for the 'abstract method' code you added. do we need tests for that?
There was a problem hiding this comment.
@CyrusNajmabadi Abstract methods already have tests. But the test was disabled. I added record tests (in addition to the existing for abstract methods), and enabled the test.
|
@CyrusNajmabadi I'm going to re-mark this as ready for review as the tests are already there. |
|
@CyrusNajmabadi Is this ready for merge? |
src/EditorFeatures/CSharp/CompleteStatement/CompleteStatementCommandHandler.cs
Outdated
Show resolved
Hide resolved
src/EditorFeatures/CSharp/CompleteStatement/CompleteStatementCommandHandler.cs
Outdated
Show resolved
Hide resolved
|
@CyrusNajmabadi Is this ready if CI passes? |
|
|
||
| if (currentNode is MethodDeclarationSyntax method && | ||
| (method.Modifiers.Any(SyntaxKind.AbstractKeyword) || method.Modifiers.Any(SyntaxKind.ExternKeyword) || | ||
| method.IsParentKind(SyntaxKind.InterfaceDeclaration))) |
There was a problem hiding this comment.
this statements is too complex to me.
CyrusNajmabadi
left a comment
There was a problem hiding this comment.
can you simplify this a bit. mixing &&'s and ||s is too confusing to me.
|
@CyrusNajmabadi Anything left? |
| if (method.Modifiers.Any(SyntaxKind.AbstractKeyword) || method.Modifiers.Any(SyntaxKind.ExternKeyword) || | ||
| method.IsParentKind(SyntaxKind.InterfaceDeclaration)) |
There was a problem hiding this comment.
❔ Is there any harm in making this unconditional?
There was a problem hiding this comment.
📝 Note that partial methods also can be declared without a body.
There was a problem hiding this comment.
@sharwell I think it will unnecessarily move the caret to the end for typos, which can be annoying.
There was a problem hiding this comment.
📝 Note that
partialmethods also can be declared without a body.
That's a good case. Going to fix it and add a test.
sharwell
left a comment
There was a problem hiding this comment.
No objection to the current expansion, but I think we should make the behavior unconditional for methods.
|
@sharwell It this ready to merge? |
No description provided.