Move the graph slice into chemvas.features.graph - #185
Merged
Conversation
The three graph modules were Qt-free policy living in the legacy ui package — the exact shape ADR 0001 says a feature should own — with one outside dependency (domain.document) and a canvas-free contract already enforced by test. No port, no bootstrap wiring, no runtime-services change was needed to move them. The shape follows the hover precedent rather than a re-export package: two real modules, no forwarding layer. algorithms.py holds the pure graph judgments and the rotation policy; __init__.py holds CanvasGraphState with the index operations, because cached_bond_in_cycle needs the state and its graph_version as a pair. The halves sit that way round because the dependency runs index-to-algorithms: giving __init__ the dependent half keeps every import top-of-file and acyclic, where the opposite orientation needs a bottom-of-file import from a partially-initialized package. ui keeps only the twelve-line graph_state_for accessor reading the runtime container field, the same leaf hover left behind. The ten modules that only use the accessor are untouched. features.* is strict, and strict earned its keep here: find_rings reused one variable first as an edge key (frozenset[int]) and then as a ring signature (frozenset[frozenset[int]]), a collision the old Any bond typing had hidden. The bindings are now separate names, and the bond, atom and point inputs are typed as small structural protocols instead of Any. The vacated ui modules join REMOVED_COMPATIBILITY_MODULES and verify_dist's forbidden list, and the new paths its required list. The four boundary checks that pin these modules by path were each re-proven against an injected violation at the new locations.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Motivation
The three graph modules — algorithms, index operations, rotation policy — are
Qt-free policy that has been living in the legacy
uipackage. That is theexact shape ADR 0001 says a feature package should own: their only outside
dependency is
domain.document, andtest_graph_algorithms_are_canvas_freealready enforces that no canvas reaches them. Unlike almost everything else in
ui, moving them costs no port, no bootstrap wiring and no runtime-serviceschange.
Changes
chemvas/features/graph/with two real modules and no re-export layer,following what the hover slice actually is — a single working module, not a
package of forwarding files.
algorithms.pyholds the pure graph judgments plus the two rotation-policyfunctions;
__init__.pyholdsCanvasGraphStatetogether with the indexoperations, since
cached_bond_in_cycleconsumes the state and itsgraph_versionas a pair. The halves sit that way round for a measured reason:the dependency runs index→algorithms, so giving
__init__the dependent halfkeeps every import top-of-file and acyclic, where the opposite orientation
needs a bottom-of-file import from a partially-initialized package — which
E402would reject anyway.ui/canvas_graph_state.pyshrinks to the twelve-linegraph_state_foraccessor reading the runtime container field, the same leaf hover left behind;
it stays enumerated in
EXPECTED_RUNTIME_STATE_ACCESSORSunchanged. The tenmodules that only use the accessor are untouched; the five function consumers
and two type consumers import from
chemvas.features.graph— the package,not its internals, which
test_feature_callers_use_package_public_apirequires and now enforces for this feature too.
features.*is under the strict profile, and strict earned its keep here:find_ringsreused one variable first as an edge key (frozenset[int]) andthen as a ring signature (
frozenset[frozenset[int]]) — a genuine typecollision the old
Any-typed bonds had hidden. The bindings are separate namesnow, and bond/atom/point inputs are typed as small structural protocols
(
BondLike,AtomCoordsLike,PointLike) rather thanAny. NoAnywasintroduced outside the accessor's conventional
canvas.The vacated
uimodule names joinREMOVED_COMPATIBILITY_MODULESandverify_dist's forbidden list; the new paths join its required list — bothfollowing the pairing the chrome move and the hover slice established.
Verification
bash scripts/check.sh— exit 0: ruff clean, formatted, mypy clean, all 300test files passing in their own processes. The 18 test files that name the
moved symbols pass together: 464 passed, 161 subtests passed.
Path-pinned checks that move with files are how a gate goes vacuous, so all
four were re-proven at the new locations: injecting
canvasinto both newfiles fails
test_graph_algorithms_are_canvas_free; a second reachability walkfails
test_seeded_graph_reachability_is_walked_in_one_place; a cache write incanvas_graph_servicefailstest_bond_cycle_cache_has_one_writer; a secondimporter fails
test_cycle_membership_is_decided_in_one_module. Exit 1 each,exit 0 restored, zero injection residue.
Packaging: a wheel and sdist from a clean
git archivetree passverify_dist.py; a tampered wheel withchemvas/ui/graph_algorithms.pyre-inserted fails it, and one with
algorithms.pyremoved fails it, each withthe exact message.
One history cost, stated:
graph_rotation_policy.pymerged intoalgorithms.py, so git records it as a delete rather than a rename and--followloses its 99-line trail there. The other two files are recognizedrenames.