fix(#1110): resolve remaining too_many_arguments clippy warnings - #1122
Merged
Merged
Conversation
Introduce focused parameter/builder structs at each call site with too many arguments, grouping only genuinely related fields per struct rather than one catch-all bag: - send_position_update: merge x,y,z into a single `to: [f32; 3]` - collision::column_hits: ColumnQuery struct - steering::steer_target: SteeringContext struct - packet_handler test helpers: CharRecord, TaskDescription structs - login/action_loop/gameplay DRY pass: ActionLoopSlots, GameplayLifecycle, ZoneEntryHandles - renderer entity_model_matrix_heading: ModelAnchor struct - App::new: drop vestigial _assets_path, split params into 5 focused structs - ModelViewerApp::new: ModelSelector, ViewerHandles structs One deliberate exception to DRY: the two `run_zone_entry_handshake` call sites in gameplay.rs each build their `ZoneEntryHandles` literal inline rather than through a shared constructor. A regression test added for #1010 (review round 1, finding 4) source-text-pins both call sites independently, specifically to catch a mutant that quietly redirects one call site to a throwaway default. A shared `from_action_loop`-style constructor would let such a mutant slip through without changing any text the pin test can see, so the small duplication stays on purpose. Verified clean: `cargo clippy --workspace --all-targets --all-features` reports zero warnings, and `cargo test` is green across all 14 workspace crates. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NxfLFobwMobnF1jKjozRKp
…ct refactor An Opus review of PR #1122 caught two doc-comment placement bugs: inserting ModelAnchor and SteeringContext ahead of the functions they configure left entity_model_matrix_heading's y_up/heading explanation and steer_target's NO-STALL INVARIANT (#382) documenting the wrong item, with the functions themselves left undocumented. Move each block back to the function it describes and give the struct its own short doc. Also: - gameplay.rs: reword the ZoneEntryHandles DRY-exception rationale. The original claim that a shared constructor "would let a mutant silently redirect one call site... without changing any text the pin can see" is not accurate -- a pin anchored on the constructor call would catch that mutant too. The real reason to keep the inline literals is narrower: it keeps the existing #1010 pin test valid as written, instead of rewriting it to match a refactor. - models.rs: update a doc's example call for entity_model_matrix_heading in render_model.rs, which this PR changed from positional args to a ModelAnchor struct literal. - floating_placement.rs: clarify that the pin test's trailing-comma normalization only reaches the end of the whole extracted argument list, not a comma nested inside a ModelAnchor struct literal -- that inner comma is pinned text. Verified: cargo clippy -p eqoxide-renderer -p eqoxide-nav -p eqoxide-net --all-targets --all-features is clean, and cargo test across the same three crates is green (eqoxide-net 451/451, including the #1010 pin test). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NxfLFobwMobnF1jKjozRKp
Owner
Author
|
An Opus subagent reviewed this PR (verified full clippy/test suite, hand-checked every changed call site against its refactored signature). Verdict: refactor itself is correct — no behavior changes, no missed call sites, no smuggled logic edits. It found two doc-comment placement bugs, now fixed in 01fb0e9:
Also fixed: the Two items flagged as legitimate but out of scope for this PR, left for a follow-up:
🤖 Generated with Claude Code |
Add HandleFixture, a small Default-derived struct owning the six
ActionLoop-owned slots ZoneEntryHandles borrows, plus a .handles()
builder that combines it with the two per-test net_health/
game_state_snapshot handles. Collapses the ~13 near-identical 10-line
ZoneEntryHandles { .. } literals in the test module down to 1-3 lines
each, and lets the now-redundant doors_bg/world_bg clone variables be
folded into the fixture's field initializers.
The two production call sites in run_gameplay_phase, and the #1010
source-text pin test that scans them, are untouched — this only
touches test-module construction sites, which the pin test never
reads.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NxfLFobwMobnF1jKjozRKp
…pSlots + GameplayLifecycle
ModelContext duplicated all 22 fields of eq_net::run_login_flow's two
parameter structs flat. Since those two structs already form an exact,
non-overlapping partition of ModelContext, nest them instead:
`ModelContext { slots: ActionLoopSlots, lifecycle: GameplayLifecycle }`.
This also makes ServerModel::run a one-line delegation (no more manual
22-field re-plumb) and makes the existing doc claim that ModelContext
"mirrors, one-for-one, the parameters run_login_flow already takes"
literally true instead of only true by field correspondence.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NxfLFobwMobnF1jKjozRKp
Owner
Author
|
Both deferred follow-ups from the Opus review are resolved:
Verified with 🤖 Generated with Claude Code |
2 tasks
djhenry
added a commit
that referenced
this pull request
Sep 15, 2026
…e merge (#1123) PR #1122 merged (f252cd9) before a second Opus review pass of its two follow-up commits landed. This carries those findings forward onto main: - Collapse the duplicated ActionLoopSlots literal in test_action_loop/test_action_loop_with_maps_dir (action_loop.rs) — the two were identical but for maps_dir. - Fix a broken intra-doc link ([`GameState`]) in model.rs's module doc, and note the coupling ModelContext now has to eq_net's own slot structs after the compose-from-ActionLoopSlots+GameplayLifecycle refactor. - Restore doc detail (the render/app thread's begin_zone_load/ finish_zone_load ownership, and the /v1/observe/nav_debug endpoint name) that was dropped from ActionLoopSlots's zone_assets/nav_debug field docs during the same refactor. - Reword HandleFixture's doc comment (gameplay.rs) — it claimed its users were "below" it (the first user is actually above it) and that HandleFixture::default() covers all ~13 call sites (only 3 do). - Fix the pin test's own doc comment, which after an earlier fix session accidentally spelled out the literal needle string (`action_loop.controller_slots()`) the test scans for, inflating its own match count from 2 to 3 and failing the test. - Align ZoneEntryHandles's field colons inside HandleFixture::handles() with the codebase's aligned-colon style used everywhere else. Verified: cargo clippy --workspace --all-targets --all-features (clean, no new warnings) and cargo test --workspace (all green) on top of main. Claude-Session: https://claude.ai/code/session_01NxfLFobwMobnF1jKjozRKp Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
This was referenced Sep 16, 2026
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.
Summary
Closes the remaining warnings from #1110 by introducing focused parameter/builder structs at each
too_many_argumentscall site, instead of one catch-all bag per function:send_position_update: merge x,y,z into a singleto: [f32; 3]collision::column_hits:ColumnQuerystructsteering::steer_target:SteeringContextstructpacket_handlertest helpers:CharRecord,TaskDescriptionstructsActionLoopSlots,GameplayLifecycle,ZoneEntryHandlesentity_model_matrix_heading:ModelAnchorstructApp::new: drop vestigial_assets_path, split params into 5 focused structsModelViewerApp::new:ModelSelector,ViewerHandlesstructsOne deliberate exception to DRY
The two
run_zone_entry_handshakecall sites ingameplay.rseach build theirZoneEntryHandlesliteral inline rather than through a shared constructor. A regression test added for #1010 (review round 1, finding 4) source-text-pins both call sites independently, specifically to catch a mutant that quietly redirects one call site to a throwaway default. A sharedfrom_action_loop-style constructor would let such a mutant slip through without changing any text the pin test can see, so this small duplication is intentional and documented in the struct's doc comment.Test plan
cargo clippy --workspace --all-targets --all-features— zero warningscargo testgreen across all 14 workspace crates (eqoxide-net: 451 passed, eqoxide-ui: 43 passed, eqoxide root: 262 passed, all others clean)🤖 Generated with Claude Code
https://claude.ai/code/session_01NxfLFobwMobnF1jKjozRKp