Remove the dead facades and give the rollback tail an owner - #187
Merged
Conversation
…roundtrips Batch 10 of the dead-facade / misplaced-code cleanup (L2-04 partial, L2-07, L2-08, L2-11). L2-07 dead forwarders (production consumers 0, evidence: unfiltered git grep two angles - attribute-call sites and word occurrences): - InsertController: load_smiles, template_point_resolvers, resolve_ring_points_for_template, resolve_regular_ring_points_for_template_bond, resolve_chair_points_for_template, resolve_boat_points_for_template, resolve_template_points_for_template_bond, bond_merge_seed - MainWindowTextStyleService.set_text_align (+ now-orphaned _TEXT_ALIGNMENTS) - StructureBuildService: ensure_ring_fills_for_model, add_linear_chain - RDKitAdapter.get_name_from_smiles Second-order ripple (targets whose only production caller was a removed forwarder): the six InsertTemplateService resolver forwarders, InsertCommitService.bond_merge_seed (module-level owner in insert_template_commit_service stays), CanvasStyleController.set_text_alignment (production alignment flows through the note controller), StructureBuildCommitter.ensure_ring_fills_for_model / add_linear_chain, RDKitImportHelper.get_name_from_smiles (+ orphaned RDKitAdapter._name_map). Wiring-only tests deleted (including one that exercised a bare mock.Mock()); real-behavior tests retargeted to the real paths (smiles_service.load_smiles, template_geometry resolvers, module-level bond_merge_seed). L2-11: capture/release/restore_scene_rect_snapshot moved from ui/transactions/scene_runtime.py to their owner ui/transactions/scene_rect.py, names unchanged; consumers (scene_runtime, history_commands, tests) now import from scene_rect. Both modules stay in the rollback-kernel list and the history-transaction acyclic cluster; scene_rect does not import scene_runtime. L2-08: the absolute-restore rollback tail (restore scene/runtime snapshot, then scene rect) was still copied verbatim at 5 sites (3 in history_commands.py, 2 in scene_runtime.py); consolidated into scene_runtime.restore_absolute_snapshots, `raise` stays at call sites. L2-04 partial: flattened two conversion-helper roundtrips that went helper -> adapter -> same helper with no side effect on the adapter hop (model_to_rdkit_with_map_tolerant in _suggest_atom_correspondence, model_to_rdkit_with_map_strict_labels in model_to_rdkit_strict_labels). The _embed_3d_molecule/_build_conversion_rdkit_mol pair is untouched: it is a documented test seam (tests patch those names on the adapter). Left for a separate decision: InsertSmilesService.load_smiles (plus its transaction-builder/rollback machinery) is now production-caller-0 but is a coherent feature with characterization tests; not removed in this batch. Verification: scripts/check.sh GATE EXIT=0 (300 test files); affected suites run directly, including test_core_rdkit_adapter under real RDKit; git diff --summary shows no mode changes.
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
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
Three kinds of code that reads like it does something and does not: forwarding
methods whose only callers are the tests that verify the forwarding, a helper
that calls itself through two hops of adapter indirection, and one eight-line
rollback tail copy-pasted five times because nothing owned it. Plus 49 lines of
scene-rect code living in the wrong module of its own package.
Changes
Twenty-three dead facade methods removed, net −488 lines. Each candidate was
re-measured on the current tree rather than taken from the survey: forwarding
methods collected by AST, then production callers counted unfiltered from two
angles — call sites and bare-word occurrences, because six candidates turned out
to be alive only as bound-method references (
kwarg=self.method) that acall-site grep alone would have missed, and those six stay. Removals cascade:
deleting a forwarder often left its target at zero production callers too, so
the unit is the chain, not the method —
bond_merge_seedthrough two layersdown to the module function that has a real internal caller and survives,
get_name_from_smilesdown to the 18-entry_name_mapdict that existed onlyto serve it. Tests that verified only the deleted wiring were deleted with it —
one asserted against a bare
mock.Mock()and could never have failed for a realreason — and tests that verified behavior through a facade were re-aimed at the
real path.
The
set_text_alignname stays in the boundary file's re-introduction ban list,which is the ban working as intended, not a leftover.
The scene-rect snapshot trio moved home.
capture/release/ restore_scene_rect_snapshotlived inscene_runtime.py— the package's largestmodule — while
scene_rect.pyis the declared owner of everything scene-rect.Same names, same package, both files already inside the rollback-kernel checks'
glob.
Two adapter round trips flattened.
RDKitConversionHelpercalled two of itsown methods as
self.adapter.X(...), which the adapter forwards straight backto the helper. Read before flattening: those two adapter methods are pure
forwarding with no
last_errorreset and noRDKitResultwrapping, and no testpatches them on the adapter. The
_embed_3d_moleculehops that look similar arenot flattened — the adapter's own comment calls them a seam, and tests
really do patch that seam.
The rollback tail got an owner. Re-measured after the note-consolidation
series, the eight-line "restore the absolute scene/runtime snapshots" tail was
still copied verbatim in five places across
history_commandsandscene_runtime. It is nowrestore_absolute_snapshots()inscene_runtime,next to the restore machinery it drives; the
raisedecisions stay at the callsites.
One thing found and deliberately not done: with its controller forwarder gone,
InsertSmilesService.load_smiles— the whole document-replacing SMILES loadwith its exact-rollback machinery — now has zero production callers. That is a
feature removal, not a cleanup, so it is recorded here and left for a separate
decision rather than folded into this diff.
Verification
bash scripts/check.sh— exit 0: ruff clean, formatted, mypy clean, all 300test files passing in their own processes. The nineteen affected test files pass
directly — 694 tests — including
test_core_rdkit_adapterunder a real RDKitinstall, which covers the flattened calls.
Every "zero consumers" claim in this diff is backed by an unfiltered two-angle
search recorded before the removal, and the survivors list shows the method that
each angle saved. No file mode changed.