refactor(core)!: replace public core module with focused facades#392
Merged
Conversation
- Make `crate::core` private and expose low-level APIs through curated `tds`, `collections`, `algorithms`, and `query` modules. - Add focused prelude/docs coverage for the new public import paths. - Update downstream-style tests and doctests to stop relying on `delaunay::core`. BREAKING CHANGE: `delaunay::core` is no longer a public module. Downstream callers should import low-level APIs from `delaunay::tds`, `delaunay::collections`, `delaunay::algorithms`, `delaunay::query`, or the matching focused preludes. Fixes #388
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughHides the crate-internal ChangesPublic namespace policy: hide core, expose curated facades
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 0 |
🟢 Coverage 88.57% diff coverage · +0.01% coverage variation
Metric Results Coverage variation ✅ +0.01% coverage variation (-1.00%) Diff coverage ✅ 88.57% diff coverage Coverage variation details
Coverable lines Covered lines Coverage Common ancestor commit (5fb2d4a) 61000 55188 90.47% Head commit (ba0435f) 61005 (+5) 55199 (+11) 90.48% (+0.01%) Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch:
<coverage of head commit> - <coverage of common ancestor commit>Diff coverage details
Coverable lines Covered lines Diff coverage Pull request (#392) 35 31 88.57% Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified:
<covered lines added or modified>/<coverable lines added or modified> * 100%
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
coderabbitai
Bot
added
documentation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CHANGELOG.md`:
- Around line 10-19: The generated CHANGELOG is missing the breaking-change note
about the namespace/visibility break in delaunay::core from PR `#392`; update the
changelog generation inputs (not the generated file) so the generator picks up
this break: ensure PR `#392` is tagged with the breaking-change label and/or add
the explicit breaking-change entry for "delaunay::core visibility break" into
the changelog metadata used by the generator (e.g., the PR release-notes body or
changelog source data) so the Unreleased section includes this namespace break
on next generation.
In `@src/lib.rs`:
- Around line 71-84: Update the crate-level documentation to stop linking or
referencing types via crate::core; replace any occurrences that expose or point
readers to the private namespace with the public curated module paths (use
crate::tds, crate::collections, crate::algorithms, crate::query and their
focused preludes) and update example/doctest/benchmark links and
cross-references that currently use crate::core::... to reference the
corresponding public symbol (e.g., public types/functions currently linked
through crate::core::TypeName should be linked as crate::tds::TypeName or
crate::collections::TypeName as appropriate), ensuring all policy and example
text and intra-doc links consistently point to the public low-level surface
instead of crate::core.
In `@tests/public_topology_api.rs`:
- Around line 11-13: Update tests that import the vertex macro from the crate
root to instead import from the recommended curated path: replace uses of
delaunay::vertex (or re-exports at crate root) with imports from
delaunay::prelude::triangulation::construction::vertex (or use a glob import
from delaunay::prelude::triangulation::construction::*). Specifically modify the
test modules mentioned (tests/tds_orientation.rs, tests/prelude_exports.rs,
tests/delaunay_public_api_coverage.rs) so they reference the vertex macro via
the construction prelude (symbol: vertex or vertex! macro) to match
tests/public_topology_api.rs and the documented guidance.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: aee10700-4d76-4166-aaf6-72fb3741bde0
📒 Files selected for processing (27)
CHANGELOG.mdCONTRIBUTING.mdREADME.mddocs/code_organization.mddocs/production_review_remediation_checklist.mdsrc/core/algorithms/incremental_insertion.rssrc/core/algorithms/locate.rssrc/core/boundary.rssrc/core/traits/facet_cache.rssrc/core/util/jaccard.rssrc/core/vertex.rssrc/lib.rssrc/triangulation.rssrc/triangulation/delaunay.rstests/README.mdtests/check_perturbation_stats.rstests/delaunay_incremental_insertion.rstests/delaunayize_workflow.rstests/proptest_convex_hull.rstests/proptest_facet.rstests/proptest_orientation.rstests/proptest_tds.rstests/proptest_vertex.rstests/public_topology_api.rstests/serialization_vertex_preservation.rstests/tds_orientation.rstests/trait_bound_ergonomics.rs
💤 Files with no reviewable changes (2)
- tests/proptest_convex_hull.rs
- src/core/algorithms/incremental_insertion.rs
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #392 +/- ##
==========================================
+ Coverage 90.46% 90.47% +0.01%
==========================================
Files 61 61
Lines 60796 60801 +5
==========================================
+ Hits 54998 55009 +11
+ Misses 5798 5792 -6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
- Report the effective batch progress cadence in the large-scale debug harness, including the canonical `DELAUNAY_BULK_PROGRESS_EVERY` fallback behavior. - Rename bulk progress tracing fields so post-initial-simplex counters are distinct from total input and final insertion statistics. - Point public docs and downstream-style API tests at curated low-level modules and construction prelude imports. - Regenerate the changelog so the core facade break appears in Unreleased. BREAKING CHANGE: Bulk progress tracing fields were renamed from `processed`, `total_vertices`, `inserted`, and `skipped` to explicit `bulk_*`, `input_vertices`, and `initial_simplex_vertices` fields.
Owner
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/lib.rs (1)
1112-1119: ⚡ Quick winPick one canonical home for point-location APIs.
querynow re-exports the samelocate/ conflict-region surface thatdelaunay::algorithmsalready owns. That gives downstream users two equally official import paths for the same low-level API and weakens the new “focused facades” story. Either keep these exports underalgorithmsonly, or documentqueryas an intentional convenience superset.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib.rs` around lines 1112 - 1119, The query module currently re-exports low-level point-location symbols (e.g., locate, locate_with_stats, find_conflict_region, ConflictError, LocateError, LocateFallback, LocateFallbackReason, LocateResult, LocateStats, extract_cavity_boundary) from crate::core::algorithms::locate which duplicates the canonical home under the delaunay::algorithms facade; remove these pub use lines from the pub mod query block so the locate APIs are only exported from the algorithms/delaunay facade (or alternatively, if you prefer query to be an intentional convenience, replace the raw re-exports with a clear module-level doc comment stating that query intentionally re-exports the locate APIs as a convenience), and update any internal imports to reference crate::core::algorithms::locate::locate (or the chosen canonical path) accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib.rs`:
- Around line 391-410: The PR made mod core private but left many public
signatures and docs referencing crate::core:: which exposes a now-private
namespace; update all public-facing references to use the public facade
re-exports instead (e.g., replace SurfaceMeasureError::FacetError(#[from]
crate::core::facet::FacetError) and all pub use crate::core::... prelude
exports) so public types and docs point to crate::tds, crate::collections,
crate::algorithms, crate::query or the corresponding re-exports in src/lib.rs;
search for occurrences of "crate::core::" across src/lib.rs,
src/triangulation.rs and docs, change them to the appropriate public facade
symbol, and ensure doc links and error type From annotations reference the
public re-exported types rather than crate::core:: symbols.
---
Nitpick comments:
In `@src/lib.rs`:
- Around line 1112-1119: The query module currently re-exports low-level
point-location symbols (e.g., locate, locate_with_stats, find_conflict_region,
ConflictError, LocateError, LocateFallback, LocateFallbackReason, LocateResult,
LocateStats, extract_cavity_boundary) from crate::core::algorithms::locate which
duplicates the canonical home under the delaunay::algorithms facade; remove
these pub use lines from the pub mod query block so the locate APIs are only
exported from the algorithms/delaunay facade (or alternatively, if you prefer
query to be an intentional convenience, replace the raw re-exports with a clear
module-level doc comment stating that query intentionally re-exports the locate
APIs as a convenience), and update any internal imports to reference
crate::core::algorithms::locate::locate (or the chosen canonical path)
accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: b3d85af4-9a94-4db6-af40-7af558766d80
📒 Files selected for processing (31)
CHANGELOG.mdCONTRIBUTING.mdREADME.mddocs/code_organization.mddocs/dev/debug_env_vars.mddocs/production_review_remediation_checklist.mdsrc/core/algorithms/incremental_insertion.rssrc/core/algorithms/locate.rssrc/core/boundary.rssrc/core/traits/facet_cache.rssrc/core/util/jaccard.rssrc/core/vertex.rssrc/lib.rssrc/triangulation.rssrc/triangulation/delaunay.rstests/README.mdtests/check_perturbation_stats.rstests/delaunay_incremental_insertion.rstests/delaunay_public_api_coverage.rstests/delaunayize_workflow.rstests/large_scale_debug.rstests/prelude_exports.rstests/proptest_convex_hull.rstests/proptest_facet.rstests/proptest_orientation.rstests/proptest_tds.rstests/proptest_vertex.rstests/public_topology_api.rstests/serialization_vertex_preservation.rstests/tds_orientation.rstests/trait_bound_ergonomics.rs
💤 Files with no reviewable changes (2)
- tests/proptest_convex_hull.rs
- src/core/algorithms/incremental_insertion.rs
- Move public documentation, doctests, and prelude exports away from the private `crate::core` namespace. - Keep point-location and conflict-region APIs canonical under `delaunay::algorithms` and `delaunay::prelude::algorithms`. - Expose repair, insertion, flip, and validation contracts through the triangulation and TDS facades. - Harden `toml-check` by passing TOML filenames to Python via argv. BREAKING CHANGE: `delaunay::query` and `delaunay::prelude::query` no longer re-export point-location and conflict-region APIs. Import `locate`, `locate_with_stats`, `find_conflict_region`, `extract_cavity_boundary`, and related locate/conflict types from `delaunay::algorithms` or `delaunay::prelude::algorithms` instead.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
justfile (1)
14-14: 💤 Low valueThe
[private]attribute placement is correct but redundant.The
[private]attribute at line 14 applies to the variable_coverage_base_args(line 15), not the recipe_ensure-actionlint. This is valid just syntax—the attribute can be placed on variables to hide them fromjust --variablesoutput. However, since the variable already starts with_, it is automatically hidden by naming convention, making the[private]attribute redundant. Consider removing it for simplicity.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@justfile` at line 14, The `[private]` attribute on the line above `_coverage_base_args` is redundant because the variable is already hidden by its leading underscore; remove the solitary `[private]` attribute line and leave the variable `_coverage_base_args` and the recipe `_ensure-actionlint` unchanged so the variable remains hidden by convention and no other attributes are affected.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@justfile`:
- Line 954: The one-liner that calls Python to validate TOML uses
open(sys.argv[1], 'rb') without closing the file; update the command string (the
Python -c invocation inside the xargs call) to open the file with a context
manager (e.g., use "with open(..., 'rb') as f: tomllib.load(f) ..." inside the
-c code) or explicitly close the file after loading so the file handle is not
leaked.
---
Nitpick comments:
In `@justfile`:
- Line 14: The `[private]` attribute on the line above `_coverage_base_args` is
redundant because the variable is already hidden by its leading underscore;
remove the solitary `[private]` attribute line and leave the variable
`_coverage_base_args` and the recipe `_ensure-actionlint` unchanged so the
variable remains hidden by convention and no other attributes are affected.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 68b8667c-3728-406e-b997-00003bab6cea
📒 Files selected for processing (11)
docs/numerical_robustness_guide.mddocs/validation.mdjustfilesrc/core/algorithms/locate.rssrc/lib.rssrc/triangulation.rssrc/triangulation/delaunay.rssrc/triangulation/delaunayize.rssrc/triangulation/flips.rssrc/triangulation/validation.rstests/allocation_api.rs
✅ Files skipped from review due to trivial changes (2)
- docs/numerical_robustness_guide.md
- src/triangulation/validation.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- src/core/algorithms/locate.rs
- src/lib.rs
- Route the exported `vertex!` macro through the canonical `tds` facade. - Harden TOML validation by loading files through a scoped Python file handle. - Regenerate changelog entries for the existing facade and diagnostics breaking changes.
This was referenced May 19, 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.
crate::coreprivate and expose low-level APIs through curatedtds,collections,algorithms, andquerymodules.delaunay::core.BREAKING CHANGE:
delaunay::coreis no longer a public module. Downstream callers should import low-level APIs fromdelaunay::tds,delaunay::collections,delaunay::algorithms,delaunay::query, or the matching focused preludes.Fixes #388