NotificationsYou must be signed in to change notification settings
Fork4.2k
Star20k

Support init accessor in CSharpSyntaxFacts#48137

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

sharwell merged 8 commits intodotnet:masterfromYoussef1313:patch-40

Oct 14, 2020

Merged

Support init accessor in CSharpSyntaxFacts#48137

sharwell merged 8 commits intodotnet:masterfromYoussef1313:patch-40

Oct 14, 2020

Conversation

Copy link

Member

Youssef1313 commentedSep 28, 2020•
edited
Loading

Prefer squash/merge.

Support init accessor in CSharpSyntaxFacts

24c66a9

Youssef1313 requested a review froma team as acode owner

September 28, 2020 23:10

Update CSharpSyntaxFacts.cs

43f31da

Youssef1313 mentioned this pull request

Sep 29, 2020

public enum DeclarationKind must have an InitAccessor member#48136

Closed

jinujoseph added Area-IDE CommunityThe pull request was submitted by a contributor who is not a Microsoft employee. labels

Sep 29, 2020

jinujoseph assignedsharwell

Sep 29, 2020

Revert

6138658

Youssef1313 commented

Sep 29, 2020

View reviewed changes

...paces/SharedUtilitiesAndExtensions/Compiler/CSharp/Services/SyntaxFacts/CSharpSyntaxFacts.cs OutdatedShow resolvedHide resolved

Update src/Workspaces/SharedUtilitiesAndExtensions/Compiler/CSharp/Se…

67f795d

…rvices/SyntaxFacts/CSharpSyntaxFacts.cs

davidwengier reviewed

Sep 29, 2020

View reviewed changes

...paces/SharedUtilitiesAndExtensions/Compiler/CSharp/Services/SyntaxFacts/CSharpSyntaxFacts.cs OutdatedShow resolvedHide resolved

Youssef1313 commented

Sep 29, 2020

View reviewed changes

...paces/SharedUtilitiesAndExtensions/Compiler/CSharp/Services/SyntaxFacts/CSharpSyntaxFacts.cs OutdatedShow resolvedHide resolved

Youssef1313 added2 commits

September 29, 2020 02:29

Apply suggestions from code review

4d8cee0

Update CSharpSyntaxFacts.cs

75d0a26

davidwengier approved these changes

Sep 29, 2020

View reviewed changes

Copy link

Member

davidwengier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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!).

Copy link

MemberAuthor

Youssef1313 commentedSep 29, 2020

@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 roslyncurrently. 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.

Copy link

Member

CyrusNajmabadi commentedSep 29, 2020

and it ends up being used for the public API SyntaxGenerator.WithAccessibility.

THis is a testable scenario :) Plz add test.

Update SyntaxGeneratorTests.cs

244f780

Copy link

Member

davidwengier commentedSep 29, 2020

It may not be needed internally for roslyn currently. But consumers of this API should get the desired behavior.

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 :)

Copy link

MemberAuthor

Youssef1313 commentedSep 29, 2020

@davidwengier Got it. Let me check what tests will fail in#48158 and update these tests for init accessor in this PR.

Youssef1313 commented

Sep 29, 2020

View reviewed changes

...paces/SharedUtilitiesAndExtensions/Compiler/CSharp/Services/SyntaxFacts/CSharpSyntaxFacts.cs Outdated

		caseSyntaxKind.PropertyDeclaration:
		caseSyntaxKind.GetAccessorDeclaration:
		caseSyntaxKind.SetAccessorDeclaration:
		caseSyntaxKind.InitAccessorDeclaration:

Copy link

MemberAuthor

Youssef1313Sep 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

@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 tosyntaxFactsService.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 onlyreturn false in the draft in a hope to see test failures (i.e. other switch arms are tested).

Youssef1313 commented

Sep 29, 2020

View reviewed changes

...paces/SharedUtilitiesAndExtensions/Compiler/CSharp/Services/SyntaxFacts/CSharpSyntaxFacts.cs Outdated

		caseSyntaxKind.PropertyDeclaration:
		caseSyntaxKind.GetAccessorDeclaration:
		caseSyntaxKind.SetAccessorDeclaration:
		caseSyntaxKind.InitAccessorDeclaration: