-
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
Share ISyntaxFacts/SyntaxGenerator with CodeStyle layer. #41376
Changes from 12 commits
2af9d32
f2ab78f
645f413
fe84de2
2dcaa98
bb9b719
56084e8
a4b4131
e037e13
9e537c7
4870d47
64bb7a7
d506fb1
87cc476
6d1ddd6
115863a
d46d186
bd2640d
07ded01
14a95f9
5e9a6ad
aa3ca17
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,11 +19,31 @@ | |
<InternalsVisibleTo Include="Microsoft.CodeAnalysis.CSharp.CodeStyle.UnitTests" /> | ||
</ItemGroup> | ||
<ItemGroup> | ||
<Compile Include="..\..\..\Workspaces\CSharp\Portable\CodeGeneration\CSharpFlagsEnumGenerator.cs" Link="CodeGeneration\CSharpFlagsEnumGenerator.cs" /> | ||
<Compile Include="..\..\..\Workspaces\CSharp\Portable\CodeGeneration\CSharpSyntaxGenerator.cs" Link="CodeGeneration\CSharpSyntaxGenerator.cs" /> | ||
<Compile Include="..\..\..\Workspaces\CSharp\Portable\CodeGeneration\ExpressionGenerator.cs" Link="CodeGeneration\ExpressionGenerator.cs" /> | ||
<Compile Include="..\..\..\Workspaces\CSharp\Portable\Extensions\ArgumentSyntaxExtensions.cs" Link="Extensions\ArgumentSyntaxExtensions.cs" /> | ||
<Compile Include="..\..\..\Workspaces\CSharp\Portable\Extensions\AssignmentExpressionSyntaxExtensions.cs" Link="Extensions\AssignmentExpressionSyntaxExtensions.cs" /> | ||
<Compile Include="..\..\..\Workspaces\CSharp\Portable\Extensions\ContextQuery\SyntaxTokenExtensions_SharedWithCodeStyle.cs" Link="Formatting\ContextQuery\SyntaxTokenExtensions_SharedWithCodeStyle.cs" /> | ||
<Compile Include="..\..\..\Workspaces\CSharp\Portable\Extensions\MemberDeclarationSyntaxExtensions_GetAttributes.cs" Link="Formatting\MemberDeclarationSyntaxExtensions_GetAttributes.cs" /> | ||
<Compile Include="..\..\..\Workspaces\CSharp\Portable\Extensions\SyntaxNodeExtensions_SharedWithCodeStyle.cs" Link="Formatting\SyntaxNodeExtensions_SharedWithCodeStyle.cs" /> | ||
<Compile Include="..\..\..\Workspaces\CSharp\Portable\Extensions\SyntaxTokenExtensions_SharedWithCodeStyle.cs" Link="Formatting\SyntaxTokenExtensions_SharedWithCodeStyle.cs" /> | ||
<Compile Include="..\..\..\Workspaces\CSharp\Portable\Extensions\ContextQuery\SyntaxTreeExtensions_Shared.cs" Link="Extensions\ContextQuery\SyntaxTreeExtensions_Shared.cs" /> | ||
<Compile Include="..\..\..\Workspaces\CSharp\Portable\Extensions\DirectiveSyntaxExtensions.cs" Link="Extensions\DirectiveSyntaxExtensions.cs" /> | ||
<Compile Include="..\..\..\Workspaces\CSharp\Portable\Extensions\DirectiveSyntaxExtensions.DirectiveInfo.cs" Link="Extensions\DirectiveSyntaxExtensions.DirectiveInfo.cs" /> | ||
<Compile Include="..\..\..\Workspaces\CSharp\Portable\Extensions\DirectiveSyntaxExtensions.DirectiveSyntaxEqualityComparer.cs" Link="Extensions\DirectiveSyntaxExtensions.DirectiveSyntaxEqualityComparer.cs" /> | ||
<Compile Include="..\..\..\Workspaces\CSharp\Portable\Extensions\DirectiveSyntaxExtensions.DirectiveWalker.cs" Link="Extensions\DirectiveSyntaxExtensions.DirectiveWalker.cs" /> | ||
<Compile Include="..\..\..\Workspaces\CSharp\Portable\Extensions\ExpressionSyntaxExtensions_Shared.cs" Link="Extensions\ExpressionSyntaxExtensions_Shared.cs" /> | ||
<Compile Include="..\..\..\Workspaces\CSharp\Portable\Extensions\ITypeSymbolExtensions.TypeSyntaxGeneratorVisitor.cs" Link="Extensions\ITypeSymbolExtensions.TypeSyntaxGeneratorVisitor.cs" /> | ||
<Compile Include="..\..\..\Workspaces\CSharp\Portable\Extensions\ITypeSymbolExtensions_Shared.cs" Link="Extensions\ITypeSymbolExtensions_Shared.cs" /> | ||
<Compile Include="..\..\..\Workspaces\CSharp\Portable\Extensions\MemberDeclarationSyntaxExtensions_GetAttributes.cs" Link="Extensions\MemberDeclarationSyntaxExtensions_GetAttributes.cs" /> | ||
<Compile Include="..\..\..\Workspaces\CSharp\Portable\Extensions\MemberDeclarationSyntaxExtensions_Shared.cs" Link="Extensions\MemberDeclarationSyntaxExtensions_Shared.cs" /> | ||
<Compile Include="..\..\..\Workspaces\CSharp\Portable\Extensions\StatementSyntaxExtensions.cs" Link="Extensions\StatementSyntaxExtensions.cs" /> | ||
<Compile Include="..\..\..\Workspaces\CSharp\Portable\Extensions\StringExtensions.cs" Link="Extensions\StringExtensions.cs" /> | ||
<Compile Include="..\..\..\Workspaces\CSharp\Portable\Extensions\SyntaxListExtensions.cs" Link="Extensions\SyntaxListExtensions.cs" /> | ||
<Compile Include="..\..\..\Workspaces\CSharp\Portable\Extensions\SyntaxNodeExtensions_SharedWithCodeStyle.cs" Link="Extensions\SyntaxNodeExtensions_SharedWithCodeStyle.cs" /> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These will certainly clash with my refactoring PR as I have split and moved bunch of these into the shared layer as well. One of us will have to deal with merge conflicts in few of files being added to shared layer now. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes. I think that is def the case. But ideally you shouldn't need to split/move these down yourself. the only splitting/moving would be for some extensions that weren't captured in this that are used by some other features? but in tha tcase, i would argue again that they shouldn't be in that big PR. we should pull them out (like the unnecessary-imports part) so that none of that is in the big PR). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The refactoring PR is now just doing this exact thing: just splitting and moving down extensions into shared layers. Your PR here is doing so for SyntaxGenerator and SyntaxFacts and my PR is doing so for other features. I think the key is we should avoid these efforts in parallel as there will be overlap that leads to big merge conflicts. I would recommend waiting on this PR till the primary refactoring PR goes in. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note: it may be hte case that that's what is going on in that PR... but it's also 8k more adds and 3k more deletes. So it's mixed in with a bunch of other stuff. We might want to consider how this could just go in, and then have the remaining stuff be teh final bit we need to review in isolation. |
||
<Compile Include="..\..\..\Workspaces\CSharp\Portable\Extensions\SyntaxTokenExtensions_SharedWithCodeStyle.cs" Link="Extensions\SyntaxTokenExtensions_SharedWithCodeStyle.cs" /> | ||
<Compile Include="..\..\..\Workspaces\CSharp\Portable\Extensions\SyntaxNodeExtensions.SingleLineRewriter.cs" Link="Extensions\SyntaxNodeExtensions.SingleLineRewriter.cs" /> | ||
<Compile Include="..\..\..\Workspaces\CSharp\Portable\Extensions\SyntaxTreeExtensions_Shared.cs" Link="Extensions\SyntaxTreeExtensions_Shared.cs" /> | ||
<Compile Include="..\..\..\Workspaces\CSharp\Portable\Extensions\SyntaxTriviaExtensions.cs" Link="Formatting\SyntaxTriviaExtensions.cs" /> | ||
<Compile Include="..\..\..\Workspaces\CSharp\Portable\Extensions\SyntaxTriviaListExtensions.cs" Link="Extensions\SyntaxTriviaListExtensions.cs" /> | ||
<Compile Include="..\..\..\Workspaces\CSharp\Portable\Formatting\CSharpFormattingOptions.cs" Link="Formatting\CSharpFormattingOptions.cs" /> | ||
<Compile Include="..\..\..\Workspaces\CSharp\Portable\Formatting\CSharpFormattingOptions.Parsers.cs" Link="Formatting\CSharpFormattingOptions.Parsers.cs" /> | ||
<Compile Include="..\..\..\Workspaces\CSharp\Portable\Formatting\CSharpSyntaxFormattingService.cs" Link="Formatting\CSharpSyntaxFormattingService.cs" /> | ||
|
@@ -56,6 +76,9 @@ | |
<Compile Include="..\..\..\Workspaces\CSharp\Portable\Formatting\Rules\SyntaxKindEx.cs" Link="Formatting\Rules\SyntaxKindEx.cs" /> | ||
<Compile Include="..\..\..\Workspaces\CSharp\Portable\Formatting\Rules\TokenBasedFormattingRule.cs" Link="Formatting\Rules\TokenBasedFormattingRule.cs" /> | ||
<Compile Include="..\..\..\Workspaces\CSharp\Portable\Formatting\Rules\WrappingFormattingRule.cs" Link="Formatting\Rules\WrappingFormattingRule.cs" /> | ||
<Compile Include="..\..\..\Workspaces\CSharp\Portable\LanguageServices\CSharpDocumentationCommentService.cs" Link="LanguageServices\CSharpDocumentationCommentService.cs" /> | ||
<Compile Include="..\..\..\Workspaces\CSharp\Portable\LanguageServices\CSharpSyntaxFactsService.cs" Link="LanguageServices\CSharpSyntaxFactsService.cs" /> | ||
<Compile Include="..\..\..\Workspaces\CSharp\Portable\LanguageServices\CSharpSyntaxKindsService.cs" Link="LanguageServices\CSharpSyntaxKindsService.cs" /> | ||
<Compile Include="..\..\..\Workspaces\CSharp\Portable\Utilities\FormattingRangeHelper.cs" Link="Formatting\FormattingRangeHelper.cs" /> | ||
</ItemGroup> | ||
<ItemGroup> | ||
|
@@ -76,4 +99,9 @@ | |
<PublicAPI Include="PublicAPI.Shipped.txt" /> | ||
<PublicAPI Include="PublicAPI.Unshipped.txt" /> | ||
</ItemGroup> | ||
<ItemGroup> | ||
<Folder Include="Extensions\ContextQuery\" /> | ||
<Folder Include="LanguageServices\" /> | ||
<Folder Include="CodeGeneration\" /> | ||
</ItemGroup> | ||
</Project> |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,6 +18,11 @@ namespace Microsoft.CodeAnalysis.Formatting | |
/// </summary> | ||
internal static class Formatter | ||
{ | ||
/// <summary> | ||
/// The annotation used to mark portions of a syntax tree to be formatted. | ||
/// </summary> | ||
public static SyntaxAnnotation Annotation { get; } = new SyntaxAnnotation(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. note: i'm taking this approach because we have code that i'm sharing that does thing like add these annotations. I could attempt to eitehr not share that code (which would mean ore shuffling around) or #ifdef it. I chose to do this for expediency as i don't really see any issue with annotations being added. |
||
|
||
/// <summary> | ||
/// Gets the formatting rules that would be applied if left unspecified. | ||
/// </summary> | ||
|
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.
We want all of these now to happen via the newly added shared projects, not with linked files.
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.
Sounds good. Let me figure out how to do that. thanks!
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.
Should we do the split first, the move all the files over into the shared projects? Looking at the projects now, it still looks like they use linked files. Seems like we can two phase this.
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.
benefit of that approach is that the sharedproject pr should have no actual code changes, just project structure changes.
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.
You should be able just create a folder named "Extensions" in the required shared projects under: https://github.com/dotnet/roslyn/tree/master/src/Workspaces/SharedUtilitiesAndExtensions. At this point you don't even need to name the files in shared projects end with
_Shared
or_SharedWithCodeStyle
suffix.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.
My refactoring PR already removes all the linked files from CodeStyle layer projects, so it might be easier if that went in?
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.
Let me update my PR with the latest sources from master, and you can see if it makes sense.
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.
Sure!