-
Notifications
You must be signed in to change notification settings - Fork 1
warp-core: EdgeRecord derives Eq #155
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
Conversation
Summary by CodeRabbitRelease Notes
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughEdgeRecord now derives Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (4)
🧰 Additional context used📓 Path-based instructions (1)**/*.rs📄 CodeRabbit inference engine (AGENTS.md)
Files:
🧠 Learnings (2)📚 Learning: 2025-12-28T23:14:28.083ZApplied to files:
📚 Learning: 2025-12-28T23:14:28.083ZApplied to files:
🔇 Additional comments (4)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/warp-core/src/record.rs (1)
17-23: Consider deriving PartialEq/Eq on NodeRecord for symmetry.For consistency and future-proofing, consider deriving
PartialEqandEqonNodeRecordas well, mirroring the treatment ofEdgeRecord. The diff logic intick_patch.rs(lines 401-402) already performs field-by-field comparison, so the derive would formalize existing semantics and enable idiomatic equality checks if needed later.🔎 Proposed consistency improvement
-#[derive(Clone, Debug)] +#[derive(Clone, Debug, PartialEq, Eq)] pub struct NodeRecord {
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
crates/warp-core/src/record.rscrates/warp-core/src/tick_patch.rsdocs/decision-log.mddocs/execution-plan.md
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Every public API across crates (warp-core, warp-ffi, warp-wasm, etc.) must carry rustdoc comments that explain intent, invariants, and usage. Treat missing docs as a failing test.
Runcargo clippy --all-targets -- -D missing_docsbefore every PR; CI will expect a zero-warning, fully documented surface.
Every source file must start with exactly:// SPDX-License-Identifier: Apache-2.0and// © James Ross Ω FLYING•ROBOTS <https://github.com/flyingrobots>
Files:
crates/warp-core/src/record.rscrates/warp-core/src/tick_patch.rs
🧠 Learnings (2)
📚 Learning: 2025-12-28T23:14:28.083Z
Learnt from: CR
Repo: flyingrobots/echo PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-28T23:14:28.083Z
Learning: When a PR touches non-doc code, update `docs/execution-plan.md` and `docs/decision-log.md` in the same PR.
Applied to files:
docs/execution-plan.md
📚 Learning: 2025-12-28T23:14:28.083Z
Learnt from: CR
Repo: flyingrobots/echo PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-28T23:14:28.083Z
Learning: Start each session by updating *Today's Intent* in `docs/execution-plan.md` and capture milestones, blockers, and decisions in the Decision Log.
Applied to files:
docs/execution-plan.md
🔇 Additional comments (4)
crates/warp-core/src/record.rs (1)
32-32: LGTM: Clean addition of equality traits.The derive correctly adds
PartialEqandEqtoEdgeRecord. All component types (EdgeId, NodeId, TypeId, and Bytes) properly implement these traits, and structural equality over all fields is the correct semantics for edge diffing in tick patches. This eliminates the now-redundant helper function and makes equality single-source-of-truth at the type level.docs/decision-log.md (1)
12-12: Well-documented decision.The decision log entry clearly captures the rationale (preventing helper/type drift) and consequences (smaller, more portable diffing code) of deriving equality on
EdgeRecord. This aligns perfectly with the implemented changes.crates/warp-core/src/tick_patch.rs (1)
437-437: Excellent: Idiomatic equality check replaces helper.The direct
!=operator leverages the derivedPartialEqimplementation onEdgeRecord, eliminating the need for a separate helper function. This is cleaner, more maintainable, and preserves the field-by-field comparison semantics of the removededge_record_eqhelper.docs/execution-plan.md (1)
38-44: Completion tracking is thorough and accurate.The execution plan correctly documents the scope, exit criteria, and evidence for the EdgeRecord equality improvement. The entry clearly states what was changed (derive traits, replace helper, remove function) and confirms validation (tests + clippy green), satisfying the Docs Guard requirement.
Based on learnings: when a PR touches non-doc code,
docs/execution-plan.mdmust be updated in the same PR.
Closes #154
PartialEq, EqforEdgeRecord.edge_record_eq(a, b)usage with idiomatica == b/a != b.tick_patch.rs.Validation:
cargo test --workspacecargo clippy --workspace --all-targets -- -D warnings -D missing_docs