feat: support to build SQLite database #22
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…alogs, updating references in documentation and code accordingly
…and package-lock.json
… and add SqlDelight documentation
… and shooting rules with corresponding queries
…l (default all); output to db/realm and db/sqlite with extracted DDL
… schema duplication
…N during DB build
…sh for improved database generation and verification
…tabase schema version
…ions for G17 CUSTOM, G17 Gen.5 MOS, P320, and G19 Gen.5 MOS
…d field specifications
…nd modify existing entries for clarity
…r real firearm counterparts in the database
…criptions and unify format for consistency
… refine shooting challenge descriptions in CSV
Contributor
|
/gemini summary |
Contributor
Summary of ChangesThis pull request significantly enhances the project's data generation capabilities by introducing support for SQLite databases in addition to the existing Realm format. This change provides greater versatility for applications consuming the catalog data. It involves a refactoring of the database build process, including a new unified build script, updated documentation, and the integration of Highlights
Changelog
Activity
|
Contributor
Summary of ChangesThis pull request significantly expands the project's data management capabilities by introducing comprehensive support for building SQLite databases, complementing the existing Realm database generation. The changes involve a refactored and unified build process, updated documentation to reflect the new database formats and source file naming conventions, and the integration of Highlights
Changelog
Activity
|
Contributor
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces support for building SQLite databases alongside the existing Realm database generation. The changes are comprehensive, including a new build script, updated documentation, new dependencies for SQLite, and the core logic for data migration from CSVs to an SQLite database. The implementation is well-structured, sharing parsing logic between both database types. My review includes a few suggestions to improve the robustness of the build scripts and enhance type safety and maintainability in the new TypeScript code.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
…eturn types for parsed CSV data
…troducing a helper function to check for statement terminators
Collaborator
Author
|
/gemini review |
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces support for building a SQLite database alongside the existing Realm database. The changes include adding a new build script, command-line options for format selection, SQLite schema definitions (.sq files), and the implementation for parsing CSVs and populating the SQLite database. The documentation has also been updated to reflect these changes.
My review focuses on improving script robustness, code clarity, and schema consistency. I've suggested using path.join for constructing file paths, simplifying the build script, and aligning column naming conventions in the new SQLite schema for better maintainability.
Comment on lines
+1
to
+13
| CREATE TABLE manufacturers( | ||
| id TEXT NOT NULL PRIMARY KEY, | ||
| shortName TEXT, | ||
| longName TEXT, | ||
| altName TEXT, | ||
| description TEXT, | ||
| url TEXT, | ||
| shortNameJa TEXT, | ||
| longNameJa TEXT, | ||
| altNameJa TEXT, | ||
| descriptionJa TEXT, | ||
| urlJa TEXT | ||
| ); |
Contributor
There was a problem hiding this comment.
For schema consistency, consider renaming the primary key column id to _id. Other tables in the schema (guns, shooting_rules, catalog_metadata) use _id as the primary key. This change would make the schema more uniform and easier to understand. You will also need to update the corresponding INSERT statement in src/sqldelight/index.ts.
CREATE TABLE manufacturers(
_id TEXT NOT NULL PRIMARY KEY,
shortName TEXT,
longName TEXT,
altName TEXT,
description TEXT,
url TEXT,
shortNameJa TEXT,
longNameJa TEXT,
altNameJa TEXT,
descriptionJa TEXT,
urlJa TEXT
);
| SELECT * FROM manufacturers; | ||
|
|
||
| selectById: | ||
| SELECT * FROM manufacturers WHERE id = ?1; |
Contributor
There was a problem hiding this comment.
If you apply the suggestion to rename the primary key to _id, this query will also need to be updated.
SELECT * FROM manufacturers WHERE _id = ?1;
Comment on lines
+110
to
123
| file = `${directory}ManufacturerCatalog.csv`; | ||
| console.log(`\nWriting manufacturer catalog from ${file}`) | ||
| success = createManufacturerCatalog(realm, `${file}`); | ||
| console.log(`${success ? "Done" : "FAILED"}`); | ||
|
|
||
| file = `${directory}RealmGunCatalog.csv`; | ||
| file = `${directory}GunCatalog.csv`; | ||
| console.log(`\nWriting gun catalog from ${file}`) | ||
| success = createGunCatalog(realm, `${file}`); | ||
| console.log(`${success ? "Done" : "FAILED"}`); | ||
|
|
||
| file = `${directory}RealmShootingRuleCatalog.csv`; | ||
| file = `${directory}ShootingRuleCatalog.csv`; | ||
| console.log(`\nWriting shooting rule catalog from ${file}`) | ||
| success = createShootingRuleCatalog(realm, `${file}`); | ||
| console.log(`${success ? "Done" : "FAILED"}`); |
Contributor
There was a problem hiding this comment.
Using path.join() to construct file paths is more robust than string concatenation. It correctly handles path separators across different operating systems and avoids potential issues like double slashes.
Suggested change
| file = `${directory}ManufacturerCatalog.csv`; | |
| console.log(`\nWriting manufacturer catalog from ${file}`) | |
| success = createManufacturerCatalog(realm, `${file}`); | |
| console.log(`${success ? "Done" : "FAILED"}`); | |
| file = `${directory}RealmGunCatalog.csv`; | |
| file = `${directory}GunCatalog.csv`; | |
| console.log(`\nWriting gun catalog from ${file}`) | |
| success = createGunCatalog(realm, `${file}`); | |
| console.log(`${success ? "Done" : "FAILED"}`); | |
| file = `${directory}RealmShootingRuleCatalog.csv`; | |
| file = `${directory}ShootingRuleCatalog.csv`; | |
| console.log(`\nWriting shooting rule catalog from ${file}`) | |
| success = createShootingRuleCatalog(realm, `${file}`); | |
| console.log(`${success ? "Done" : "FAILED"}`); | |
| file = path.join(directory, 'ManufacturerCatalog.csv'); | |
| console.log(`\nWriting manufacturer catalog from ${file}`) | |
| success = createManufacturerCatalog(realm, file); | |
| console.log(`${success ? "Done" : "FAILED"}`); | |
| file = path.join(directory, 'GunCatalog.csv'); | |
| console.log(`\nWriting gun catalog from ${file}`) | |
| success = createGunCatalog(realm, file); | |
| console.log(`${success ? "Done" : "FAILED"}`); | |
| file = path.join(directory, 'ShootingRuleCatalog.csv'); | |
| console.log(`\nWriting shooting rule catalog from ${file}`) | |
| success = createShootingRuleCatalog(realm, file); | |
| console.log(`${success ? "Done" : "FAILED"}`); |
Comment on lines
+46
to
+48
| `INSERT INTO manufacturers (id, shortName, longName, altName, description, url, shortNameJa, longNameJa, altNameJa, descriptionJa, urlJa) | ||
| VALUES (@_id, @shortName, @longName, @altName, @description, @url, @shortNameJa, @longNameJa, @altNameJa, @descriptionJa, @urlJa)` | ||
| ); |
Contributor
There was a problem hiding this comment.
This INSERT statement needs to be updated if you rename the id column to _id in the manufacturers table for consistency, as suggested in sqldelight/catalog/Manufacturer.sq.
Suggested change
| `INSERT INTO manufacturers (id, shortName, longName, altName, description, url, shortNameJa, longNameJa, altNameJa, descriptionJa, urlJa) | |
| VALUES (@_id, @shortName, @longName, @altName, @description, @url, @shortNameJa, @longNameJa, @altNameJa, @descriptionJa, @urlJa)` | |
| ); | |
| `INSERT INTO manufacturers (_id, shortName, longName, altName, description, url, shortNameJa, longNameJa, altNameJa, descriptionJa, urlJa) | |
| VALUES (@_id, @shortName, @longName, @altName, @description, @url, @shortNameJa, @longNameJa, @altNameJa, @descriptionJa, @urlJa)` |
Comment on lines
+116
to
+129
| const manufacturers = ParseCSV.parseCsvFile( | ||
| path.normalize(`${directory}ManufacturerCatalog.csv`), | ||
| RealmManufacturerCatalog.schema as any | ||
| ) as (IManufacturerCatalog | null)[]; | ||
|
|
||
| const guns = ParseCSV.parseCsvFile( | ||
| path.normalize(`${directory}GunCatalog.csv`), | ||
| RealmGunCatalog.schema as any | ||
| ) as (IGunCatalog | null)[]; | ||
|
|
||
| const shootingRules = ParseCSV.parseCsvFile( | ||
| path.normalize(`${directory}ShootingRuleCatalog.csv`), | ||
| RealmShootingRuleCatalog.schema as any | ||
| ) as (IShootingRuleCatalog | null)[]; |
Contributor
There was a problem hiding this comment.
For robustness and cross-platform compatibility, it's better to use path.join() to construct file paths instead of string concatenation with path.normalize().
Suggested change
| const manufacturers = ParseCSV.parseCsvFile( | |
| path.normalize(`${directory}ManufacturerCatalog.csv`), | |
| RealmManufacturerCatalog.schema as any | |
| ) as (IManufacturerCatalog | null)[]; | |
| const guns = ParseCSV.parseCsvFile( | |
| path.normalize(`${directory}GunCatalog.csv`), | |
| RealmGunCatalog.schema as any | |
| ) as (IGunCatalog | null)[]; | |
| const shootingRules = ParseCSV.parseCsvFile( | |
| path.normalize(`${directory}ShootingRuleCatalog.csv`), | |
| RealmShootingRuleCatalog.schema as any | |
| ) as (IShootingRuleCatalog | null)[]; | |
| const manufacturers = ParseCSV.parseCsvFile( | |
| path.join(directory, "ManufacturerCatalog.csv"), | |
| RealmManufacturerCatalog.schema as any | |
| ) as (IManufacturerCatalog | null)[]; | |
| const guns = ParseCSV.parseCsvFile( | |
| path.join(directory, "GunCatalog.csv"), | |
| RealmGunCatalog.schema as any | |
| ) as (IGunCatalog | null)[]; | |
| const shootingRules = ParseCSV.parseCsvFile( | |
| path.join(directory, "ShootingRuleCatalog.csv"), | |
| RealmShootingRuleCatalog.schema as any | |
| ) as (IShootingRuleCatalog | null)[]; |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.