Support init accessor in CSharpSyntaxFacts#48137
Conversation
…rvices/SyntaxFacts/CSharpSyntaxFacts.cs
davidwengier
left a comment
There was a problem hiding this comment.
This change makes sense, but I don't see too much value in aggressively updating all of these APIs without known consuming use cases, or tests (or both!).
|
@davidwengier I looked at the usages of one of these changes, and it ends up being used for the public API SyntaxGenerator.WithAccessibility. It may not be needed internally for roslyn currently. But consumers of this API should get the desired behavior. I haven't confirmed whether the other change ends up being used in a public API or not. |
THis is a testable scenario :) Plz add test. |
I don't disagree, but there could also be issues if things light up in areas that they were never intended to, or otherwise don't support. Hopefully, of course, the existing test coverage validates that :) |
|
@davidwengier Got it. Let me check what tests will fail in #48158 and update these tests for init accessor in this PR. |
| case SyntaxKind.PropertyDeclaration: | ||
| case SyntaxKind.GetAccessorDeclaration: | ||
| case SyntaxKind.SetAccessorDeclaration: | ||
| case SyntaxKind.InitAccessorDeclaration: |
There was a problem hiding this comment.
@davidwengier I removed both GetAccessorDeclaration and InitAccessorDeclaration in a draft. There are no test failures. So the current tests, unfortunately, doesn't cover those.
The usages that depend on this method are in:
- CodeLens\CodeLensReferencesService.cs (there is a call to
syntaxFactsService.IsDeclaration(node)) - CodeRefactorings\SyncNamespace\AbstractChangeNamespaceService.cs (there is a call to
.DescendantNodes(n => !syntaxFacts.IsDeclaration(n)))
Unlike the other change, I'm not able to guess the effect of this change.
I'll try to change the method to only return false in the draft in a hope to see test failures (i.e. other switch arms are tested).
| case SyntaxKind.PropertyDeclaration: | ||
| case SyntaxKind.GetAccessorDeclaration: | ||
| case SyntaxKind.SetAccessorDeclaration: | ||
| case SyntaxKind.InitAccessorDeclaration: |
There was a problem hiding this comment.
I don't think it's a good idea to update this one without knowing the change it introduce.
This whole method isn't covered in the tests. See how #48158 is green.
|
@CyrusNajmabadi @davidwengier Is this ready to merge? |

Prefer squash/merge.