Skip to content
This repository was archived by the owner on Apr 27, 2026. It is now read-only.

Variable declaration addition#3152

Merged
jonsequitur merged 11 commits into
dotnet:mainfrom
bleaphar:main
Aug 31, 2023
Merged

Variable declaration addition#3152
jonsequitur merged 11 commits into
dotnet:mainfrom
bleaphar:main

Conversation

@bleaphar

Copy link
Copy Markdown
Contributor

No description provided.

{
_tokens = new HttpLexer(_sourceText, _syntaxTree).Lex();

var unmatchedComments = new List<HttpCommentNode>();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'd probably just call this comments. They're matched, they're just not parented yet. Or maybe something like commentsToPrepend?


}

private HttpVariableValueNode? ParseVariableExpression()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
private HttpVariableValueNode? ParseVariableExpression()
private HttpVariableValueNode? ParseVariableValue()

{
break;
}
}*/

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Feel free to delete leftover comments.

return null;
}

private HttpSyntaxToken? GetNextNodeType()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This name looks inaccurate. How is this different from GetNextSignificantToken?

if (MoreTokens() && CurrentToken is { Kind: HttpTokenKind.Punctuation } and { Text: "#" })
{
return true;
} else if (MoreTokens() && CurrentToken is { Kind: HttpTokenKind.Punctuation } and { Text: "/" } &&

@jonsequitur jonsequitur Aug 25, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You can refactor this to dedupe the MoreTokens call.

{
return true;
}
else if (MoreTokens() && CurrentToken is { Kind: HttpTokenKind.Punctuation } and { Text: "/" } &&

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You can refactor this to dedupe the MoreTokens call.

{
public HttpVariableDeclarationNode? DeclarationNode { get; private set; }
public HttpVariableAssignmentNode? AssignmentNode { get; private set; }
public HttpVariableValueNode? ExpressionNode { get; private set; }

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Calling this property ValueNode is probably a better match to the property's type name at this point.

yield return diagnostic;
}

yield return base.CreateDiagnostic("Variable name expected.");

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This looks like a bug (and presumably a missing test?) unless there's a condition around it, e.g. if (Span.Length ==0).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I don't fully get what you mean here

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This will always return a diagnostic even if the node is perfectly valid.

@jonsequitur
jonsequitur enabled auto-merge (squash) August 26, 2023 00:14
auto-merge was automatically disabled August 30, 2023 22:29

Head branch was pushed to by a user without write access

@jonsequitur
jonsequitur merged commit a081474 into dotnet:main Aug 31, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants