Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a PebbleDB backend alongside the existing SQLite store, adds a factory and migration command, and updates the CLI, tests, and docs to support choosing between backends.
- Define and wire up a
SubtitleStoreinterface withOpenStorefactory - Implement
PebbleStore(with tests) and aMigrateToPebblefunction (with tests) - Update SQL store, CLI commands, docs, and dependencies for backend selection and migration
Reviewed Changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/database/store_factory.go | Add OpenStore factory to select SQLite or Pebble by flag |
| pkg/database/store.go | Define the SubtitleStore interface |
| pkg/database/pebble.go | Implement PebbleStore with JSON storage, List, Delete, etc. |
| pkg/database/pebble_test.go | Unit tests for PebbleStore insert/list and delete |
| pkg/database/migrate.go | Add MigrateToPebble to copy data from SQLite to PebbleDB |
| pkg/database/migrate_test.go | Test the migration of records from SQLite to Pebble |
| pkg/database/database.go | Refactor SQL store into SQLStore, add new fields and methods |
| pkg/database/database_test.go | Update legacy tests to new InsertSubtitle signature |
| cmd/translate.go | Use OpenStore and new SubtitleStore API in translate cmd |
| cmd/root.go | Add --db-backend flag and bind to Viper |
| cmd/migrate.go | Add migrate Cobra command for SQLite→Pebble migration |
| cmd/history.go | Switch history command to use OpenStore and interface methods |
| cmd/delete.go | Switch delete command to use OpenStore and interface methods |
| go.mod | Add Pebble and related deps (cockroachdb/pebble, google/uuid) |
| docs/TECHNICAL_DESIGN.md | Document new backend option and updated schema functions |
| README.md | Update usage docs for --db-backend and migrate command |
| CHANGELOG.md | Record new PebbleDB backend version bump |
| TODO.md | Note completion of PebbleDB work |
Comments suppressed due to low confidence (1)
pkg/database/database.go:125
- There are no unit tests for the new
SQLStoremethods (InsertSubtitle,ListSubtitles,DeleteSubtitle). Add tests that exercise those methods and verify the schema and data round-trip.
func (s *SQLStore) InsertSubtitle(rec *SubtitleRecord) error {
| } | ||
|
|
||
| // OpenSQLStore opens or creates an SQLite database and returns a SQLStore. | ||
| func OpenSQLStore(path string) (*SQLStore, error) { |
There was a problem hiding this comment.
OpenSQLStore never calls initSchema to create or migrate the SQLite schema, so tables won’t exist on first use. Add a call to initSchema(s.db) after opening the database.
| // InsertSubtitle stores a new subtitle record with associated metadata. | ||
| func InsertSubtitle(db *sql.DB, file, video, lang, service, release string, embedded bool) error { | ||
| _, err := db.Exec(`INSERT INTO subtitles (file, video_file, release, language, service, embedded, created_at) VALUES (?, ?, ?, ?, ?, ?, ?)`, | ||
| file, video, release, lang, service, boolToInt(embedded), time.Now()) | ||
| return err | ||
| } |
There was a problem hiding this comment.
[nitpick] The free function InsertSubtitle overlaps with the SQLStore method and has a long argument list, which can be confusing. Consider deprecating or renaming legacy functions and consolidating to the SubtitleStore interface.
| // InsertSubtitle stores a new subtitle record with associated metadata. | |
| func InsertSubtitle(db *sql.DB, file, video, lang, service, release string, embedded bool) error { | |
| _, err := db.Exec(`INSERT INTO subtitles (file, video_file, release, language, service, embedded, created_at) VALUES (?, ?, ?, ?, ?, ?, ?)`, | |
| file, video, release, lang, service, boolToInt(embedded), time.Now()) | |
| return err | |
| } | |
| // Deprecated: Use SQLStore.InsertSubtitle instead. | |
| // func InsertSubtitle(db *sql.DB, file, video, lang, service, release string, embedded bool) error { | |
| // _, err := db.Exec(`INSERT INTO subtitles (file, video_file, release, language, service, embedded, created_at) VALUES (?, ?, ?, ?, ?, ?, ?)`, | |
| // file, video, release, lang, service, boolToInt(embedded), time.Now()) | |
| // return err | |
| // } |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Summary
SubtitleStoreinterface with SQLite and Pebble implementations--db-backendflagTesting
go test ./...https://chatgpt.com/codex/tasks/task_e_6845d83256808321bf7000209224bf9f