-
Notifications
You must be signed in to change notification settings - Fork 1
Make generated output files optional #46
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
Conversation
7f0f622 to
f0f8faa
Compare
| @@ -1,10 +0,0 @@ | |||
| -- ** Database generated with pgModeler (PostgreSQL Database Modeler). | |||
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 file still exists but its empty because there are no diffs.
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 makes generated output files (SQL, PNG, SVG) optional and configurable through the trek.yaml configuration file, addressing issue #37. When output types are defined in the config, files are generated at customizable or default paths. The initial migration logic has also been updated to generate diffs instead of directly copying SQL files.
Changes:
- Added
outputconfiguration section to control which file types (SQL, PNG, SVG) are generated - Refactored SQL export to use a temporary directory, only copying to output path if enabled
- Added SVG export capability and made PNG export conditional
Reviewed changes
Copilot reviewed 8 out of 14 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
internal/configuration/config.go |
Added Output, OutputFile types and GetOutputPath() method to support optional output files |
internal/templates/trek.yaml.tmpl |
Updated template to include empty output section for SQL and SVG by default |
internal/pgmodeler.go |
Renamed functions (consistent casing) and added PgmodelerExportSVG() function |
cmd/generate.go |
Modified generation logic to conditionally create output files and use temp directory for SQL |
tests/output/trek.yaml, example/trek.yaml |
Added output configuration sections |
tests/output/*.gen.{sql,svg}, example/*.gen.{sql,svg} |
New generated output files |
tests/output/migrations/001_init.up.sql, example/migrations/001_init.up.sql |
Updated migration files with different content structure |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
f0f8faa to
6b4904e
Compare
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
Copilot reviewed 8 out of 14 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
6b4904e to
eaa31b8
Compare
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
Copilot reviewed 8 out of 14 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| - name: {{.}}{{end}} | ||
| output: | ||
| sql: {} | ||
| svg: {} |
Copilot
AI
Jan 15, 2026
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 trek.yaml template now includes output configuration for sql and svg but not png. Consider documenting this behavior or adding a comment in the template to explain that png can also be configured if needed. This would help users understand all available output options.
| svg: {} | |
| svg: {} | |
| # Note: PNG output can also be configured here if needed, for example: | |
| # png: {} |
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.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
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
Copilot reviewed 8 out of 14 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| sqlContent, err = os.ReadFile(tmpSQLPath) | ||
| if err != nil { | ||
| return "", fmt.Errorf("failed to read sql file: %w", err) | ||
| } | ||
| err = os.WriteFile(filepath.Join(wd, sqlPath), sqlContent, 0o644) //nolint:gosec |
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.
Why not copy the file without read/write?
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.
How? There is no os.Copy right?
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.
io.Copy, but I guess in doesn't really matter given the amount of data we're handling here.
| sql: {} | ||
| svg: {} |
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.
For the example it would be nice to have one with an explicit path.
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 thought about that, but the example is generated via trek init which doesn't have env vars to set these.. I don't think adding env vars there makes sense. I think instead we should improve documentation in general. And/or maybe the example dir shouldn't be generated?
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.
Yeah maybe get rid of the example, since it's empty anyway it doesn't have that much value and it isn't even useful as a starting point, since we have trek init.
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.
Lets pick that up in a separate story/PR; to improve docs in general.
provokateurin
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.
Please regenerate the tests
eaa31b8 to
98fc879
Compare
|
Rebased. Also re-ran tests but it didn't make any changes. |
Shouldn't the config file change after? |
|
The config file already has the new outputs right? |
Fixes #37