Skip to content

Refactoring #190

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

Merged
merged 2 commits into from
Jun 4, 2025
Merged

Refactoring #190

merged 2 commits into from
Jun 4, 2025

Conversation

ra1028
Copy link
Owner

@ra1028 ra1028 commented Jun 3, 2025

Pull Request Type

  • Bug fix
  • New feature
  • Refactoring
  • Documentation update
  • Chore

@Copilot Copilot AI review requested due to automatic review settings June 3, 2025 11:55
Copy link

@Copilot Copilot AI left a 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 refactoring replaces the old Graph and StoreState types with explicit dependencies and children collections on AtomStore and updates Snapshot accordingly.

  • Inline the dependency graph and state fields into AtomStore
  • Update Snapshot to hold dependencies/children instead of a Graph
  • Revise tests to use the new store fields and updated Snapshot initializer

Reviewed Changes

Copilot reviewed 16 out of 16 changed files in this pull request and generated no comments.

Show a summary per file
File Description
Sources/Atoms/Snapshot.swift Replaced graph with dependencies & children; updated description and graph generation
Sources/Atoms/AtomStore.swift Removed Graph/StoreState; added dependencies, children, caches, states, subscriptions, subscribed, scopes
Sources/Atoms/Core/StoreContext.swift Switched all store.state.* and store.graph.* to direct fields
Sources/Atoms/Core/TopologicalSort.swift Updated traversal to use children and subscriptions properties
Sources/Atoms/Core/Graph.swift Removed the now-unused Graph struct
Tests/AtomsTests/**/* Updated snapshots, assertions, and store references to use new fields
Comments suppressed due to low confidence (4)

Sources/Atoms/Snapshot.swift:26

  • The description method omits the subscriptions field. Consider adding - subscriptions: \(subscriptions) so the textual snapshot includes all its properties.
        - caches: \(caches)

Sources/Atoms/Snapshot.swift:78

  • [nitpick] The local variable children shadows the children property, which may confuse readers. Consider renaming the local binding (e.g. to childKeys).
            if let children = children[key] {

Sources/Atoms/Core/TopologicalSort.swift:22

  • [nitpick] This local children shadows the store's children property. Renaming it (e.g. childKeys) would improve clarity.
            if let children = children[key] {

Tests/AtomsTests/SnapshotTests.swift:26

  • This Snapshot call is missing the children parameter; the initializer requires both dependencies and children. Add a children: [...] argument to match the new signature.
              dependencies: [

@ra1028 ra1028 merged commit 844ffeb into main Jun 4, 2025
6 checks passed
@ra1028 ra1028 deleted the refactoring branch June 4, 2025 10:00
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