-
Notifications
You must be signed in to change notification settings - Fork 25
feat: ast node text serialization for pretty printing #289
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
base: main
Are you sure you want to change the base?
Conversation
sgulseth
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would argue against calling it a formatter. the groq ast doesn't contain whitespaces or comments, so you can't really use it as other formatters(prettier, gofmt, etc etc). It also requires groq query to be a valid query since we don't support partial parsing.
I would rename the api group to serializer, and call the format serializeString(). We can also put it into groq-js/beta so we don't need to commit to the api, and can change it a bit if needed.
Co-authored-by: Sindre Gulseth <[email protected]>
| @@ -0,0 +1,535 @@ | |||
| import t from 'tap' | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some tests here are testing that X in gives X out. Could we change it to something like:
const queries: string[] = [
"*",
"@",
....
]
for (const query of queries) {
t.test('query: ' + query, async (t) => {
const expected = parse(query)
const result = serializeString(tree)
t.strictSame(parse(result), expected)
})
}
makes it easier to add a bunch of queries to test and that we generate the same identical queries, kinda similar to round-trip only that we compare the ast and not the generated string
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your idea is much better since it focuses on the AST and wont break without having the query already formatted. No-brainer. I don't have a bunch of queries handy to add just yet, though. I looked into the generated assertions already in the project, but they seem to be individual node assertions not necessarily transferrable to testing the serializer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding a collection of groq queries derived from our documentation
Co-authored-by: Sindre Gulseth <[email protected]>
Added GROQ query fixtures from Sanity doc articles to improve test coverage. Fixed various serialization bugs that were surfaced by these real-world queries, ensuring AST parsing compatibility before and after serialization.
|
Changing to draft while refactoring beta/experimental |
- Rename beta.ts to experimental.ts for clearer naming - Make experimental module completely self-contained by re-exporting all main module functionality - Fix TypeScript type compatibility issues between different entry points - Update package.json exports and typesVersions to support experimental module - Update test imports to use new experimental module name This resolves type incompatibility issues where types from different entry points were seen as incompatible by TypeScript, even when they were functionally identical.
Description
This PR introduces a node serializer which can be used as a pretty printer. The serializer takes parsed GROQ AST nodes and outputs clean, consistently formatted GROQ query strings.
The serializer implements the complete GROQ specification for string literals, operators, objects, arrays, and all query constructs. It handles proper indentation for nested structures, consistent spacing around operators, and correct escaping of string literals according to the GROQ spec.
Key features:
What to review
Reviewers should focus on:
src/serializer/- verify that all GROQ node types are handled correctlyformatValue()- ensure it matches the GROQ specification for string literalstest/serializer.test.ts- added tests covering many serializing scenariosparse(serialize(parse(query)))produces consistent resultsTest the formatter by:
{serialize}fromgroq-js/betaparse()serialize(tree)Testing
Added test coverage including:
There may be parts of the spec uncovered by current serializing tests!