Skip to content
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

fix(grit): broaden GritQL parser support #4978

Open
wants to merge 5 commits into
base: next
Choose a base branch
from

Conversation

morgante
Copy link
Contributor

Summary

Fix a few more issues with the GritQL parser/formatter that failed on complex patterns from https://github.com/getgrit/stdlib

Test Plan

Spec tests are included

@morgante morgante changed the base branch from main to next January 27, 2025 03:36
@github-actions github-actions bot added A-Parser Area: parser A-Formatter Area: formatter A-Tooling Area: internal tools L-Grit Language: GritQL labels Jan 27, 2025
@morgante morgante marked this pull request as ready for review January 27, 2025 04:18
Copy link

codspeed-hq bot commented Jan 27, 2025

CodSpeed Performance Report

Merging #4978 will not alter performance

Comparing morgante:more-grit-patterns (8f3474c) with next (5184067)

Summary

✅ 95 untouched benchmarks

Copy link
Member

@ematipico ematipico left a comment

Choose a reason for hiding this comment

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

It seems we never construct the new node that was defined in this PR

Comment on lines +93 to +94
// Comments inside the JS are not allowed
let mut brace_count = 0;
Copy link
Member

Choose a reason for hiding this comment

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

If comments aren't allowed inside JS functions, maybe we should create a new lexer context, where we raise a parsing diagnostic every time we encounter a comment. At the moment it doesn't seem we do

p.expect(T!['}']);
} else {
parse_curly_predicate_list(p).ok();
}

Present(m.complete(p, GRIT_FUNCTION_DEFINITION))
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we use the new JavaScript function node here?

node: &GritJavascriptFunctionDefinition,
f: &mut GritFormatter,
) -> FormatResult<()> {
format_verbatim_node(node.syntax()).fmt(f)
Copy link
Member

Choose a reason for hiding this comment

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

We should implement the formatting while we're at it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Formatter Area: formatter A-Parser Area: parser A-Tooling Area: internal tools L-Grit Language: GritQL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants