-
Notifications
You must be signed in to change notification settings - Fork 79
Add positional pattern support for gel generate queries #1308
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
Add positional pattern support for gel generate queries #1308
Conversation
- Implement CLI parsing to collect positional patterns before flags - Add patterns field to CommandOptions interface - Integrate globby for pattern matching in getMatches() - Add comprehensive test for pattern functionality - Fix TypeScript ESM config to support dynamic imports (es2015 -> es2020) Usage: gel generate queries 'pattern1' 'pattern2' --flags Example: gel generate queries 'src/**/*.edgeql' --target ts
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.
Pull Request Overview
This PR adds support for glob pattern-based file selection in the EdgeDB query generator. It enables developers to selectively generate queries by specifying glob patterns as positional arguments, enhancing workflow efficiency for large projects.
- Adds glob pattern support using globby library for selective query generation
- Implements schema protection that automatically excludes migration and fixup files
- Updates TypeScript module target from ES2015 to ES2020 for dynamic imports
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/generate/tsconfig.esm.json | Updates module target to ES2020 to support dynamic imports |
| packages/generate/src/queries.ts | Adds pattern matching logic with globby integration and schema exclusion |
| packages/generate/src/commandutil.ts | Extends CommandOptions interface with patterns field |
| packages/generate/src/cli.ts | Implements positional argument parsing for pattern collection |
| packages/generate/package.json | Moves globby to runtime dependencies and adds tsx as dev dependency |
| packages/generate/test/cli.test.ts | Adds comprehensive test coverage for pattern matching functionality |
| docs/queries.rst | Updates documentation with pattern usage examples and features |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
packages/generate/src/queries.ts
Outdated
| if (!pattern.endsWith(".edgeql")) { | ||
| // Special case for current directory | ||
| if (pattern === ".") { | ||
| return "./**/*.edgeql"; |
Copilot
AI
Sep 2, 2025
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.
The pattern ./**/*.edgeql may not behave consistently across different operating systems. Consider using **/*.edgeql instead for better cross-platform compatibility.
| return "./**/*.edgeql"; | |
| return "**/*.edgeql"; |
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 don't personally know a ton about path portability with globby, but if this is true, I 👍 this 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.
Per globby documentation this is the way to go for portability.
Note that glob patterns can only contain forward-slashes, not backward-slashes, so if you want to construct a glob pattern from path components, you need to use path.posix.join() instead of path.join().
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 think the comment was about dropping the leading ./ right, not about the slashes?
|
Points addressed |
|
FYI: Nightly failing due to change of scoping in nightly Gel server. See geldata/gel-go#391 |
…nore options - Replace manual pattern normalization with globby's expandDirectories option - Replace post-processing schema filtering with native ignore patterns - Reduces code complexity by ~25 lines while maintaining all functionality - Better performance for pattern cases through more targeted file traversal - All existing tests continue to pass
packages/generate/src/queries.ts
Outdated
| ) { | ||
| if (patterns && patterns.length > 0) { | ||
| // Use globby to match files with the provided patterns | ||
| const { globby } = await import("globby"); |
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.
Since this would not be conditional anymore, we can just move this into a normal import.
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.
That requires setting type module because globby is pure esm. Happy to do that too, which shouldn't matter if this is just used as a bin exclusively.
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.
ahh, ok, let's not bother doing that yet then: we can make that a separate effort to not hold this up longer than it needs to.
scotttrinh
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.
Looks great, and thanks for all of the hard work getting this PR working! 🥇
| while (args.length && !args[0]!.startsWith("-")) { | ||
| positionalArgs.push(args.shift()!); | ||
| } |
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.
This breaks if you accidentally place an option between two positional args, right? Seems fine for now: we should upgrade to a more robust argument parser at some point, but wanted to make a note of it while I was re-reading.
Add positional pattern support to gel generate queries
Summary
Enables selective query generation by accepting glob patterns as positional
arguments.
Due to the bump of es2015 module target to es2020 this probably warrants a major release.
Usage
Key Features
Changes
Testing
8 test cases covering pattern matching, schema exclusion, directory traversal,
absolute paths, and edge cases.
Backward Compatibility
✅ Fully backward compatible - no patterns defaults to existing behavior