fix for comment following headers #3173
Conversation
| @@ -81,6 +81,11 @@ public void Add(HttpBodyNode node) | |||
| AddInternal(node); | |||
| } | |||
|
|
|||
| public void Add(HttpCommentNode node) | |||
There was a problem hiding this comment.
Is this needed? So far, the explicit Add methods were only required for nodes that appear as properties...
There was a problem hiding this comment.
They're needed for all node types that can be added.
There was a problem hiding this comment.
We had discussed this and decided to include the Add method for comments on the base HttpSyntaxNode class (because otherwise implementing ParseLeadingCommentsAndWhitespace becomes cumbersome),
HttpSyntaxNode already has an Add method that handles comments. Looks like it has an additional addBefore parameter - perhaps we can make that parameter optional with a default value and remove the explicit overload in HttpRequestNode above?
There was a problem hiding this comment.
We had discussed this and decided to include the Add method for comments on the base HttpSyntaxNode class (because otherwise implementing ParseLeadingCommentsAndWhitespace becomes cumbersome),
Since comments aren't valid in all syntax comments, adding this Add method on HttpSyntaxNode would be misleading.
| AddCommentsIfAny(comments, requestNode, addBefore: false); | ||
| } | ||
|
|
||
| if (ParseComment() is { } commentAfterHeaders) |
There was a problem hiding this comment.
Does this work if there are multiple comments OR multiple comments separated by new lines?
Also looks like this won't handle any leading whitespace before # or //.
I still feel this is better handled via ParseLeadingCommentsAndWhitespace in the Parse method for whatever construct follows. However, I am yet to review your responses in the earlier PR - so please ignore if you already addressed this there :)
There was a problem hiding this comment.
One other thought is that perhaps ParseTrailingWhitespace can be updated to also parse trailing comments (and perhaps that will handle the case where ParseLeadingCommentsAndWhitespace can't handle the above comment (i.e., in the case where there is no construct that follows after the header section).
(Also wondering how we currently parse a document that only contains comments but no requests i.e., under what construct do such comments end up being added today?
It may be a good idea to add (skipped) tests for the above cases assuming we don't have them already.)
There was a problem hiding this comment.
We don't have a consistent pattern here but the same problems applies as we encountered with comments generally. Parsing leading whitespace in that way e.g. within ParseComment requires gathering it up and only adding it to the tree if we actually encounter a comment node. Otherwise, it needs to be stored and applied to the next node type that we do encounter.
Leading whitespace before comments is not supported in the old parser for either kind of comment prefix (# or //).
We don't support //-prefixed comments in the new parser at all.
There was a problem hiding this comment.
We don't support //-prefixed comments in the new parser at all.
Based on the following code in the parser, // should be supported.
There was a problem hiding this comment.
The current design where we can only parse a single comment in these spots seems problematic. It is also odd that we don't support leading whitespace before comments just at these spots (we do support leading whitespace for any comments parsed via ParseLeadingCommentsAndWhitespace in other spots).
I can take a closer look and take a stab at improving this later. Will sync up offline once I have a PR / proposal.
There was a problem hiding this comment.
Based on the following code in the parser, // should be supported.
This might work correctly now. I believe there were lookaheads in places that didn't take // into account.
There was a problem hiding this comment.
The current design where we can only parse a single comment in these spots seems problematic.
This is just a bug. I haven't added combinatorial tests for all known syntax combinations in order to avoid trying to fix too many issues in a single massive PR. That and //-prefixed comments are up next.
No description provided.