-
Notifications
You must be signed in to change notification settings - Fork 12
fixup/ci: audit and machete #18
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
base: master
Are you sure you want to change the base?
fixup/ci: audit and machete #18
Conversation
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 refactors database initialization and migration logic while updating CI workflows and dependency versions.
- Refactored imports and builder APIs for Postgres and SQLite stores
- Removed migration flag from the SQLite store constructor and updated test cleanup logic
- Revised CI workflow steps and dependency versions
Reviewed Changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
src/test.rs | Updated import paths and adjusted cleanup logic using drop_tables (truncate behavior) |
src/sqlite.rs | Removed migration flag from Store constructors |
src/postgres.rs and src/pg_store_builder.rs | Refactored builder API for Postgres store creation and global network initialization |
src/lib.rs | Updated module exposure and removed redundant builder definitions |
examples/bdk_sqlx_postgres.rs | Adjusted usage of PgStoreBuilder to align with refactored API |
audit.toml, Cargo.toml, workflows | Updated dependency versions and CI workflow configurations |
Files not reviewed (1)
- Justfile: Language not supported
Comments suppressed due to low confidence (1)
src/test.rs:141
- The function name drop_tables is now misleading since it truncates table contents instead of dropping the tables. Consider renaming it to truncate_tables for improved clarity.
pub async fn drop_tables() -> anyhow::Result<()> {
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 aims to streamline the migration process and update CI configurations while refactoring the store builders for both Postgres and SQLite backends. Key changes include:
- Introducing the migrate! macro in TestStore and removing the migrate flag for the SQLite store.
- Extracting and refactoring Postgres store builder functionality into a separate module.
- Updating dependency versions and modifying the CI workflow to include additional tooling (cargo audit and cargo machete).
Reviewed Changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
src/test.rs | Updated migration invocation; removed deprecated drop tables function. |
src/sqlite.rs | Removed use of migration flag and its associated logic from the SQLite store. |
src/postgres.rs | Refactored error handling to use unified crate::Result and added conflict clauses. |
src/pg_store_builder.rs | Introduced a new PgStoreBuilder with improved connection handling. |
src/lib.rs | Removed obsolete PgStoreBuilder definitions and reorganized network setup. |
examples/bdk_sqlx_postgres.rs | Updated builder usage to reflect the new PgStoreBuilder API. |
audit.toml, Cargo.toml, .github/workflows/rust.yml | Updated dependency versions and CI configuration. |
Files not reviewed (1)
- Justfile: Language not supported
Comments suppressed due to low confidence (1)
src/pg_store_builder.rs:98
- Using initialize_network in a boolean context may mask initialization errors. Consider explicitly handling and propagating the error from initialize_network to provide clearer diagnostics.
if self.network.and_then(|n| initialize_network(n).ok()).is_none() {
src/test.rs
Outdated
migrate!("migrations/postgres"); | ||
Box::pin(store.read()) | ||
} | ||
TestStore::Sqlite(store) => { | ||
migrate!("migrations/sqlite"); |
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 migrate! macro is invoked without error handling; ensure that any migration failures are either logged or properly propagated to prevent silent errors during test initialization.
migrate!("migrations/postgres"); | |
Box::pin(store.read()) | |
} | |
TestStore::Sqlite(store) => { | |
migrate!("migrations/sqlite"); | |
if let Err(e) = migrate!("migrations/postgres") { | |
error!("Failed to run migrations for Postgres: {:?}", e); | |
return Box::pin(async { Err(BdkSqlxError::MigrationError(e.to_string())) }); | |
} | |
Box::pin(store.read()) | |
} | |
TestStore::Sqlite(store) => { | |
if let Err(e) = migrate!("migrations/sqlite") { | |
error!("Failed to run migrations for Sqlite: {:?}", e); | |
return Box::pin(async { Err(BdkSqlxError::MigrationError(e.to_string())) }); | |
} |
Copilot uses AI. Check for mistakes.
No description provided.