Skip to content

Conversation

@CyrusNajmabadi
Copy link
Member

Followup to @sharwell's great work

Before:

image

After:

image

@CyrusNajmabadi CyrusNajmabadi requested a review from a team as a code owner March 26, 2020 20:45
@CyrusNajmabadi CyrusNajmabadi requested a review from sharwell March 26, 2020 20:45
else
{
newLeadingTrivia.Add(trivia);
}
Copy link
Member Author

Choose a reason for hiding this comment

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

this was a required and unpleasant change to make. it has to do with how the formatting engine subtly treats the source produced by ILSpy different from normal MAS source. Specifically in MAS we generate like this:

int Foo {get;}//
// this is the comment for Bar
int Bar {get;}

With our formatting rule the formatter converts this to the expected:

int Foo {get;}
//
// this is the comment for Bar
int Bar {get;}

However, with decompilation we start with:

int Foo {get;}

//
// this is the comment for Bar
int Bar {get;}

And no matter what rules i specify the formatting engine will not remove that blank line. I've debugged through the deep bowels of teh formatting engine, and i cannot make any sort of sense of what it is doing here. it literally has an embedded rule deep in the core engine that simply overries any actual provided rules here and change things to be 'preserve lines' for comments. I do not want to change that code, so i'm taking hte much simple approach here of just not having the blank line trivia at the start be jammed in. This seems to have no negative consequences anywhere, and is trivial to do.

new CSharpParenthesesReducer(),
new CSharpDefaultExpressionReducer());

private class FormattingRule : AbstractMetadataFormattingRule
Copy link
Member Author

Choose a reason for hiding this comment

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

moved to its own file.


namespace Microsoft.CodeAnalysis.MetadataAsSource
{
internal abstract class AbstractMetadataFormattingRule : AbstractFormattingRule
Copy link
Member Author

Choose a reason for hiding this comment

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

this is just a move.

using var _ = ArrayBuilder<SyntaxTrivia>.GetInstance(out var newLeadingTrivia);

foreach (var trivia in node.GetLeadingTrivia())
foreach (var trivia in node.GetLeadingTrivia().SkipWhile(t => !t.IsKind(SyntaxKind.SingleLineDocumentationCommentTrivia)))
Copy link
Member Author

Choose a reason for hiding this comment

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

this was a required and unpleasant change to make. it has to do with how the formatting engine subtly treats the source produced by ILSpy different from normal MAS source. Specifically in MAS we generate like this:

int Foo {get;}//
// this is the comment for Bar
int Bar {get;}

With our formatting rule the formatter converts this to the expected:

int Foo {get;}
//
// this is the comment for Bar
int Bar {get;}

However, with decompilation we start with:

int Foo {get;}

//
// this is the comment for Bar
int Bar {get;}

And no matter what rules i specify the formatting engine will not remove that blank line. I've debugged through the deep bowels of teh formatting engine, and i cannot make any sort of sense of what it is doing here. it literally has an embedded rule deep in the core engine that simply overries any actual provided rules here and change things to be 'preserve lines' for comments. I do not want to change that code, so i'm taking hte much simple approach here of just not having the blank line trivia at the start be jammed in. This seems to have no negative consequences anywhere, and is trivial to do.

@CyrusNajmabadi
Copy link
Member Author

Closing out. We're going to go with #42850 as that allows the members to still be spaced (when expanded) but be combined when collapsed.

@CyrusNajmabadi CyrusNajmabadi deleted the decompileSpacing branch April 11, 2021 18:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant