Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR implements folder include functionality for the \i directive, allowing users to include entire folders of SQL files instead of specifying individual files. The feature addresses GitHub issue #47 by enabling \i folder/ syntax to recursively include all .sql files in a folder in alphabetical order.
Key changes:
- Modified
\idirective to support folder paths ending with/ - Added recursive folder processing with depth-first search traversal
- Files within folders are processed alphabetically, with subdirectories processed recursively
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| testdata/include/main.sql | Updated test data to use folder includes for types/ and procedures/ |
| testdata/include/expected_full_schema.sql | Updated expected output reflecting alphabetical ordering of files within folders |
| internal/include/processor_test.go | Added comprehensive test cases for folder include functionality |
| internal/include/processor.go | Implemented core folder include logic with recursive processing |
| docs/workflow/modular-schema-files.mdx | Added documentation explaining folder include feature and usage |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| // Returns the resolved path and a flag indicating if it's a folder | ||
| func (p *Processor) resolveIncludePath(includePath string, currentDir string) (string, bool, error) { | ||
| // Check if this is a folder path (ends with /) | ||
| isFolder := strings.HasSuffix(includePath, "/") |
There was a problem hiding this comment.
Consider trimming the trailing slash from includePath when isFolder is true to avoid potential path resolution issues. The current implementation may cause problems when constructing file paths later.
| isFolder := strings.HasSuffix(includePath, "/") | |
| isFolder := strings.HasSuffix(includePath, "/") | |
| if isFolder { | |
| includePath = strings.TrimSuffix(includePath, "/") | |
| } |
| // Ensure the file content ends with a newline for proper concatenation | ||
| if !strings.HasSuffix(fileContent, "\n") { | ||
| fileContent += "\n" | ||
| } |
There was a problem hiding this comment.
This newline handling logic is duplicated from the single file processing path. Consider extracting this into a helper method to avoid code duplication and ensure consistent behavior.
| } | ||
|
|
||
| // Check that folder include directive was replaced | ||
| if strings.Contains(result, "\\i types/") { |
There was a problem hiding this comment.
The string literal \\i types/ uses double backslash which may be confusing. Consider using a raw string literal or a constant to make the intent clearer.
| if strings.Contains(result, "\\i types/") { | |
| if strings.Contains(result, `\i types/`) { |
1 participant
Fix #47