Changed: Moves TopologyEdit to triangulation::flips#192
Conversation
Moves the `TopologyEdit` trait to `triangulation::flips` and renames it to `BistellarFlips`. This change involves updating imports and references throughout the codebase and documentation to reflect the new location and name of the trait.
WalkthroughThe PR renames and relocates the public editing trait from Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client as Client Code
participant Trait as BistellarFlips (trait)
participant DT as DelaunayTriangulation
participant TDS as Triangulation (TDS)
participant Core as CoreAlgorithms
Client->>Trait: request_bistellar_flip(k, args)
Trait->>DT: (impl on DelaunayTriangulation) check & delegate
DT->>TDS: delegate flip call (self.tri.flip_*)
alt Triangulation impl
Trait->>TDS: call flip method (flip_k1_insert / flip_k2 / apply_bistellar_flip_dynamic)
end
TDS->>Core: perform low-level flip logic / validations
Core-->>TDS: flip result or FlipError
TDS-->>Trait: return outcome (or FlipError::UnsupportedDimension)
Trait-->>Client: propagate result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
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 |
coderabbitai
bot
added
breaking change
documentation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/triangulation/flips.rs (1)
258-264:⚠️ Potential issue | 🟡 Minor
D - 1inflip_k3_inverse_from_trianglewill panic at compile-time or underflow ifD == 0.Both impls pass
D - 1toapply_bistellar_flip_dynamic. IfDis 0, this would underflow. This is likely impossible given the domain (triangulations are always ≥ 1D), but a compile-time assert or doc comment noting the minimumDrequirement would make the contract explicit.Also applies to: 301-307
🧹 Nitpick comments (3)
docs/code_organization.md (1)
174-176: Directory ordering:triangulation/appears beforetopology/in the tree.Alphabetically,
topology/sorts beforetriangulation/(o<r). If you regenerate the tree with the command shown at line 30, the output should havetopology/first. Consider swapping these to keep the tree consistent with the auto-generation tip.Proposed fix
-│ ├── triangulation/ -│ │ └── flips.rs │ ├── topology/ │ │ ├── characteristics/ │ │ │ ├── euler.rs │ │ │ └── validation.rs │ │ ├── spaces/ │ │ │ ├── euclidean.rs │ │ │ ├── spherical.rs │ │ │ └── toroidal.rs │ │ ├── traits/ │ │ │ └── topological_space.rs │ │ └── manifold.rs +│ ├── triangulation/ +│ │ └── flips.rssrc/triangulation/flips.rs (1)
224-265: The twoBistellarFlipsimplementations are nearly identical — consider a macro or helper to reduce duplication.The
TriangulationandDelaunayTriangulationimpls differ only in how they accesstds/kernel(self.tdsvsself.tri.tds). This is a common delegation pattern, but with 6 methods the duplication is noticeable. A declarative macro could generate both impls from a single definition. That said, this is a stylistic preference and the current code is perfectly clear.Also applies to: 267-308
src/lib.rs (1)
892-911: Newtriangulationmodule is well-structured.The module cleanly wraps the
flipssubmodule and provides convenient re-exports ofDelaunayTriangulationandTriangulationfor discoverability.One observation: the nested
triangulation::prelude::flips(lines 901-906) seems redundant given that users will typically import viadelaunay::prelude::triangulation::flips::*or directly viadelaunay::triangulation::flips::*. If no external consumer needs the intermediate prelude, it could be removed to simplify the module tree. Not a blocker.
Coverage summary from CodacySee diff coverage on Codacy
Coverage variation details
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: Diff coverage details
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: See your quality gate settings Change summary preferences |
Codecov Report❌ Patch coverage is
❌ Your patch status has failed because the patch coverage (9.09%) is below the target coverage (50.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #192 +/- ##
==========================================
+ Coverage 63.05% 63.08% +0.03%
==========================================
Files 46 46
Lines 10118 10119 +1
==========================================
+ Hits 6380 6384 +4
+ Misses 3738 3735 -3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Streamlines the prelude modules to provide clearer and more focused exports for common triangulation tasks. This change affects import statements in documentation and examples, requiring more specific paths for certain types.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tests/delaunay_incremental_insertion.rs`:
- Line 565: Remove the unused import of the Coordinate trait: delete the `use
delaunay::geometry::Coordinate;` line, since the test only constructs points via
`Point::new()` and calls inherent methods like `.coords()` which don't require
the `Coordinate` trait to be in scope; ensure no other code in this file relies
on `Coordinate` before committing.
🧹 Nitpick comments (2)
src/lib.rs (2)
1155-1161:prelude::topology::validationdelegates totopology::prelude::validation.This adds an extra indirection layer (
prelude::topology::validation→topology::prelude::validation→ actual items). It works, but the nesting is a bit deep. Consider whether flattening to a direct re-export fromcrate::topology::*would simplify the path.
985-997:topology::prelude::validationmodule is only consumed byprelude::topology::validation.This intermediate
topology::preludemodule exists solely so thatprelude::topology::validationcan re-export it. If no external consumer directly usesdelaunay::topology::prelude::validation, you could simplify by havingprelude::topology::validationre-export directly fromcrate::topology::*.
Removes the redundant topology validation prelude module. Moves its contents into the main topology prelude, simplifying module structure and reducing code duplication. This change internally refactors the prelude modules for better organization.
1 participant
Moves the
TopologyEdittrait totriangulation::flipsand renames it toBistellarFlips.This change involves updating imports and references throughout the codebase and documentation to reflect the new location and name of the trait.