Skip to content

refactor(edda-ledger): split sqlite_store.rs into sub-modules (GH-376)#382

Merged
fagemx merged 2 commits intomainfrom
refactor/GH-376-split-sqlite-store-v2
Mar 27, 2026
Merged

refactor(edda-ledger): split sqlite_store.rs into sub-modules (GH-376)#382
fagemx merged 2 commits intomainfrom
refactor/GH-376-split-sqlite-store-v2

Conversation

@fagemx
Copy link
Copy Markdown
Owner

@fagemx fagemx commented Mar 27, 2026

Summary

  • Split the 7,003-line sqlite_store.rs into a sqlite_store/ directory module with 9 files using Rust's split-impl-block pattern
  • Each sub-module adds its own impl SqliteStore block for a cohesive concern (events, decisions, schema, etc.)
  • Pure code move — no behavior, SQL queries, or public API changes

New module layout

File Lines Concern
mod.rs ~70 + 3,375 tests Struct definition, open/create, Drop, all tests
types.rs 251 All public row structs and enums
mappers.rs 414 Row mapping functions, materialization helpers
schema.rs 806 Schema constants (V1-V12), migrations, version management
events.rs 585 Append, iterate, get, find, refs, chain verification
decisions.rs 396 Active decisions, timelines, domains, cross-project sync
village.rs 326 Village stats and recurring pattern detection
dependencies.rs 448 Dep graph, causal chain, outcome metrics
entities.rs 402 CRUD for device tokens, suggestions, bundles, briefs, snapshots

Key design choices

  • conn field visibility changed from private to pub(crate) so sub-modules can access it
  • All mapper/helper functions are pub(super) for cross-module use within sqlite_store
  • Tests remain centralized in mod.rs since they exercise multiple concerns
  • lib.rs re-exports unchanged — external callers see no difference

Closes #376

Test plan

  • cargo fmt --check -p edda-ledger passes
  • cargo clippy -p edda-ledger -- -D warnings passes
  • cargo test -p edda-ledger — 159 tests pass (identical to baseline)
  • cargo test --workspace — all workspace tests pass
  • cargo clippy --workspace -- -D warnings passes
  • No public API changes (same re-exports in lib.rs)
  • Each code sub-module under 806 lines
  • sqlite_store.rs no longer exists (replaced by directory)

🤖 Generated with Claude Code

Convert the 7K-line sqlite_store.rs into a directory module with 9 files:
- mod.rs: struct definition, open/create, Drop, tests
- types.rs: all public row structs and enums
- mappers.rs: row mapping functions and materialization helpers
- schema.rs: schema constants (V1-V12), migrations, version management
- events.rs: append, iterate, get, find, refs, chain verification
- decisions.rs: active decisions, timelines, domains, cross-project sync
- village.rs: village stats and recurring pattern detection
- dependencies.rs: dep graph, causal chain, outcome metrics
- entities.rs: CRUD for device tokens, suggestions, bundles, briefs, snapshots

Pure code move — no behavior, SQL, or public API changes. All 159 tests
pass identically. The conn field visibility changes to pub(crate) so
sub-modules can access it via self.conn.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@fagemx
Copy link
Copy Markdown
Owner Author

fagemx commented Mar 27, 2026

Code Review: PR #382 (Round 1)

Summary

Clean refactor splitting the 7K-line sqlite_store.rs into 9 sub-module files. The workspace compiles, clippy passes with zero warnings, and all 159 edda-ledger tests pass. Module organization is logical and the pub(super) / pub(crate) visibility choices are mostly correct.

Key Findings

Critical Issues (P0)

None.

High Priority (P1)

  1. detect_trend_direction accidentally made publicvillage.rs:308 declares this as pub fn and mod.rs:16 re-exports it via pub use village::detect_trend_direction. In the original sqlite_store.rs, this was a private function (fn detect_trend_direction). This contradicts the PR description's claim of "no public API changes." Should be pub(super) instead of pub, and the re-export removed.

  2. conn field widened from private to pub(crate)mod.rs:31 declares pub(crate) conn: Connection. The original had conn: Connection (private). In Rust, child modules can access private fields of their parent module's structs, so pub(crate) is unnecessarily broad. This exposes the raw SQLite connection to the entire edda-ledger crate (e.g., ledger.rs, sync.rs). Should remain private for encapsulation.

Testing Review

Coverage

All existing tests were preserved in mod.rs and pass (159/159). No new behavior was introduced, so no new tests are needed.

Convention Compliance

  • Tests correctly use super::mappers::time_now_rfc3339 and super::schema::* to access internal items.
  • Test helper tmp_db() pattern is preserved.
  • No testing violations found.

Testing Verdict: Adequate

Verdict: Changes Requested

Fixing P1 issues and will re-review.


Round 1 of automated review-fix loop

…irection

- Revert conn field from pub(crate) to private (child modules can access parent's private fields)
- Change detect_trend_direction from pub to pub(super) to match original private visibility
- Remove unnecessary re-export of detect_trend_direction from mod.rs

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@fagemx
Copy link
Copy Markdown
Owner Author

fagemx commented Mar 27, 2026

Code Review: PR #382 (Round 2) — LGTM 🎉

All P0 and P1 issues have been resolved.

Summary

Clean refactor splitting sqlite_store.rs (7,003 lines) into a sqlite_store/ directory module with 9 files. After fixes:

  • Module visibility: All internal sub-modules are private (mod), only types is pub mod for external type access. Internal functions/types use pub(super) correctly.
  • No API changes: conn field remains private, detect_trend_direction is pub(super) (matching original private visibility). All lib.rs re-exports unchanged.
  • All re-exports intact: 12 types re-exported via pub use types::* and pub use sqlite_store::{...} in lib.rs.
  • Build clean: cargo check, cargo clippy -- -D warnings, and all 159 edda-ledger tests pass.

Verdict: LGTM ✅

No critical or high-priority issues remaining. This PR is ready for merge.


Completed after 2 round(s) of automated review-fix loop

@fagemx fagemx merged commit 5321f81 into main Mar 27, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

refactor(ledger): split sqlite_store.rs into sub-modules

1 participant