-
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
Support abstract methods and records in complete statement command #48128
Conversation
src/EditorFeatures/CSharpTest/CompleteStatement/CSharpCompleteStatementCommandHandlerTests.cs
Show resolved
Hide resolved
@@ -47,6 +47,9 @@ internal static int MethodM(int a, int b) | |||
[InlineData("abstract void M(object o = default(object$$))", "abstract void M(object o = default(object))")] | |||
[InlineData("abstract void M(object o = default($$object))", "abstract void M(object o = default(object))")] | |||
[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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
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.
ok!
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this statements is too complex to me.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❔ Is there any harm in making this unconditional?
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.
📝 Note that partial
methods also can be declared without a body.
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.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📝 Note that
partial
methods also can be declared without a body.
That's a good case. Going to fix it and add a test.
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.
No objection to the current expansion, but I think we should make the behavior unconditional for methods.
@sharwell It this ready to merge? |
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.
thanks!
No description provided.