Rewrite WhitespaceRewriter #2604
Conversation
| span.ChunkGenerator is MarkupChunkGenerator || | ||
| span.ChunkGenerator == SpanChunkGenerator.Null); | ||
| } | ||
| } |
There was a problem hiding this comment.
I'm assuming that this is just a rename
| newBlock.Children.Add(node); | ||
| } | ||
| return newBlock.Build(); | ||
| } |
There was a problem hiding this comment.
I'm assuming that this is just a rename
| // Replace the existing code block with the whitespace literal | ||
| // followed by the rewritten code block (with the code whitespace removed). | ||
| result = result.ReplaceNode(codeBlock, new SyntaxNode[] { whitespaceLiteral, rewritten }); | ||
| } |
There was a problem hiding this comment.
The idiomatic way to implement this is to override Visit(CSharpCodeBlock ) and do your work in there.
There was a problem hiding this comment.
Ah NVM, I see that you're doing operating on result. NP.
| { | ||
| newBlock.Children.Add(node); | ||
| var literalContent = GetContent(literal); | ||
| if (literalContent.All(char.IsWhiteSpace)) |
There was a problem hiding this comment.
Should this be string.IsWhitespace(? This is a really inefficient way to implement that kind of check.
There was a problem hiding this comment.
This line was copy-paste from old code. Will replace.
| public override SyntaxNode Visit(SyntaxNode node) | ||
| { | ||
| return block.Type == BlockKindInternal.Expression && Parent != null; | ||
| var result = node; |
There was a problem hiding this comment.
This isn't really needed. You could use node instead becuase the result isn't really ever returned.
There was a problem hiding this comment.
I don't follow. It is used later in base.Visit(result).
There was a problem hiding this comment.
I'm saying you could just reassign node. This is a nitpick anyways, you do you
ajaybhargavb
force-pushed
the
ajbaaska/taghelper-parser
branch
from
343bee4 to
8101726
Compare
| { | ||
| newBlock.Children.Add(node); | ||
| var literalContent = GetContent(literal); | ||
| if (literalContent != null && string.IsNullOrWhiteSpace(literalContent)) |
There was a problem hiding this comment.
string.IsNullOrWhiteSpace should take care of literalContent being null.
There was a problem hiding this comment.
In theory, you could get away with just checking if it has non-whitespace content no? node.DescendantNodes().Where(n => n.IsToken).Cast<SyntaxToken>().Any(t => !string.IsNullOrWhitespace(t.Content))
There was a problem hiding this comment.
string.IsNullOrWhiteSpace should take care of literalContent being null.
Not really. I don't want the if block to execute when literalContent is null.
ajaybhargavb
force-pushed
the
ajbaaska/taghelper-parser
branch
from
8101726 to
87842cc
Compare
NTaylorMullen
left a comment
NTaylorMullen
left a comment
There was a problem hiding this comment.
Everything else looks good, clarified most feedback offline.
* Rewrite WhitespaceRewriter * Rename CSharpExpression to CSharpExplicitExpression
* Rewrite WhitespaceRewriter * Rename CSharpExpression to CSharpExplicitExpression
* Rewrite WhitespaceRewriter * Rename CSharpExpression to CSharpExplicitExpression
* Rewrite WhitespaceRewriter * Rename CSharpExpression to CSharpExplicitExpression
* Rewrite WhitespaceRewriter * Rename CSharpExpression to CSharpExplicitExpression
I added a bunch of syntax manipulation APIs as part of 623d743 (Didn't add that here because it might be too much noise)
ConditionalAttributeCollapser(Needs some changes at the IR lowering level)In the future, we could also think about removing the need for WhitespaceRewriter entirely.