Skip to content

refactor(sqlite-storage): rename SqliteOrigin variants for clarity#4814

Open
NathanFlurry wants to merge 1 commit intoengine-stabilize/sqlite-invalidate-v1-passthroughfrom
engine-stabilize/sqlite-origin-rename
Open

refactor(sqlite-storage): rename SqliteOrigin variants for clarity#4814
NathanFlurry wants to merge 1 commit intoengine-stabilize/sqlite-invalidate-v1-passthroughfrom
engine-stabilize/sqlite-origin-rename

Conversation

@NathanFlurry
Copy link
Copy Markdown
Member

Description

Please include a summary of the changes and the related issue. Please also include relevant motivation and context.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes.

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

Copy link
Copy Markdown
Member Author

NathanFlurry commented Apr 27, 2026

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

This stack of pull requests is managed by Graphite. Learn more about stacking.

@claude
Copy link
Copy Markdown

claude Bot commented Apr 27, 2026

Code Review

Overview

Pure enum variant rename across the SqliteOrigin type:

  • NativeCreatedOnV2
  • MigratingFromV1MigrationFromV1InProgress

All 21 deletions/21 additions are mechanical renames — no behavioral changes. The PR touches sqlite-storage types, pegboard production code, and the associated test files.


Assessment

Naming clarity — improvement. Both new names are more precise:

  • CreatedOnV2 makes the semantic explicit: this actor's storage was originally created on the v2 format, not migrated from v1. Native was ambiguous (native to what?).
  • MigrationFromV1InProgress reads unambiguously as a transient state. MigratingFromV1 was fine but the noun-phrase form (...InProgress) is more consistent with how persistent state enums tend to be named.

Wire-format safety. SqliteOrigin is serialized with serde_bare, which encodes enum variants by their zero-based ordinal index, not by name. The ordinal positions are unchanged:

  • index 0: NativeCreatedOnV2
  • index 1: MigratedFromV1 (unchanged)
  • index 2: MigratingFromV1MigrationFromV1InProgress

No existing persisted data will be misread after this change.

Coverage. Every call site has been updated: production match arms in actor_sqlite.rs and open.rs, all test fixture helpers, and all assertion sites. One test function name was also updated (decode_db_head_defaults_legacy_rows_to_created_on_v2_origin). Nothing was missed.

Comment maintenance. The inline comment in open.rs:99 that listed the variant names was updated in lockstep — good.


Minor suggestions

  • The MigratedFromV1 variant (already completed migration) uses past-tense. For consistency it could be MigratedFromV1MigrationFromV1Complete, matching the ...InProgress / ...Complete pair pattern with MigrationFromV1InProgress. Not a blocker, just worth considering while the rename is open.
  • PR description checklist items are all unchecked — this is a draft, so fine for now, but worth filling in before requesting review.

Verdict

Looks good. Clean, safe, mechanical refactor with no missed sites and no serialization risk. Ready to merge once the draft status is resolved.

@NathanFlurry NathanFlurry marked this pull request as ready for review April 27, 2026 10:02
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.

1 participant