Repository navigation
Add edgeToVertexes function - #1272
maringantis wants to merge 1 commit into
Conversation
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes. ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughThe change adds the public ChangesDirected edge endpoint vertices
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Feature Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 22.22% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 9 functions across 3 files. (2 skipped: 2… | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Linked Issues check The PR implements the requested edgeToVertexes(H3Index edge, H3Index *v1, H3Index *v2)API for directed edges [#1071] [#421].vertex.cvalidates the directed edge, obtains its origin and direction…Out of Scope Changes check The reported changes remain connected to [ #1071] and [#421]. The public declaration and changelog document the new API. The tests andfuzzerVertexesupdate verify and exercise the new behavior. No u…Title check The title clearly and concisely identifies the main change: adding the edgeToVertexes function. Description check The description directly explains the edgeToVertexes API, implementation, tests, and scope. It is relevant to the changeset. Full details: Linked Issues check
-
Explanation
The PR implements the requested
edgeToVertexes(H3Index edge, H3Index *v1, H3Index *v2)API for directed edges [#1071] [#421].vertex.cvalidates the directed edge, obtains its origin and direction, selects the start vertex withvertexNumForDirection, selects the next vertex in CCW order, and converts both vertices withcellToVertex. This matches the requested right-hand ordering in [#1071]. The header declaration uses theedgeToVertexesAPI group. Tests cover a hexagon edge, pentagon edges, invalid inputs, and boundary endpoint ordering. The optional discussion of undirected-edge support in [#421] does not require implementation of undirected edges.
Full details: Out of Scope Changes check
-
Explanation
The reported changes remain connected to [
#1071] and [#421]. The public declaration and changelog document the new API. The tests andfuzzerVertexesupdate verify and exercise the new behavior. No unrelated source or test behavior is identified in the supplied change summary or reviewed file evidence.
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 22.22% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 9 functions across 3 files. (2 skipped: 2 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
✨ Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 help to get the list of available commands.
|
|
dc8b3dc to
eb4309a
Compare
Add edgeToVertexes(H3Index edge, H3Index *v1, H3Index *v2), which returns the two vertex H3Indexes at the ends of a directed edge. Previously, getting an edge's vertexes required manually reconstructing them via cellToVertexes/directedEdgeToBoundary. The implementation mirrors the vertex-selection logic already used by directedEdgeToBoundary: it validates the edge, finds the start vertex number for the edge's direction via vertexNumForDirection, takes the next vertex number (mod 5 or 6 for pentagons/hexagons) as the end vertex, and converts both to H3 vertex indexes via the existing cellToVertex. The function is declared in h3api.h.in with a @defgroup block following the same convention as cellToVertex/cellToVertexes, so it is automatically picked up by the generated `binding-functions` list used by language bindings - no separate export list needed. Adds unit tests in testVertex.c covering a valid hexagon edge (vertexes valid, distinct, members of the origin cell, and matching the edge boundary endpoints), a pentagon edge, and invalid inputs (H3_NULL, a cell passed as an edge, an edge with an invalid direction, and a pentagon's K-axis edge). Also wires the function into the existing vertex fuzzer. Closes uber#1071. Also resolves uber#421, the older duplicate of the same request. The API signature matches what nrabinowitz scoped in the issue thread, described as "dead simple" to implement. A prior automated PR (uber#1086) took a similar approach but was abandoned after a CI formatting nit; this change was built and verified with clang-format-14 (matching CI) to avoid the same issue.
eb4309a to
4b4996c
Compare
|
Re: CodeRabbit's docstring-coverage warning on this PR — checked it against the diff: |
Summary
Adds
H3Error edgeToVertexes(H3Index edge, H3Index *v1, H3Index *v2), which returns the two vertex H3Indexes at the ends of a directed edge.Today, getting an edge's two vertexes requires manually reconstructing them via
cellToVertexes/directedEdgeToBoundary. This function makes that a single call, as requested in #1071 (and the older duplicate, #421).The signature matches what @nrabinowitz scoped in the #421 thread, where
edgeToVertexeswas called out as something that "should be dead simple" to implement (as opposed to the hardervertexesToDirectedEdgeinverse, which is out of scope here).Implementation
edgeToVertexeslives insrc/h3lib/lib/vertex.c, next tocellToVertex/cellToVertexes, and reuses the same vertex-selection logic already used bydirectedEdgeToBoundary:isValidDirectedEdge.vertexNumForDirection.cellToVertex.The function is declared in
src/h3lib/include/h3api.h.inwith a@defgroup edgeToVertexesdoc block, following the same convention as the neighboringcellToVertex/cellToVertexesdeclarations. Becausemake binding-functionsderives its list of public API functions from these@defgroupmarkers, the new function is automatically picked up there for language bindings - no separate export list needed.Tests
Added to
src/apps/testapps/testVertex.c:edgeToVertexes: a valid hexagon edge returns two valid, distinct vertexes that belong to the origin cell's vertex set and match the first/last points ofdirectedEdgeToBoundary.edgeToVertexes_pentagon: exercises all of a pentagon's valid edges.edgeToVertexes_invalid:H3_NULL, a cell passed in place of an edge, an edge with an invalid direction digit, and a pentagon's deleted K-axis edge all returnE_DIR_EDGE_INVALID.Also added a call into the existing
fuzzerVertexesharness.Formatting / CI note
A prior automated attempt at this (#1086, now closed/abandoned) implemented essentially the same idea but was never merged after a CI formatting nit from @isaacbrodsky (clang-format-14 vs. a different local version). To avoid repeating that, I built with
clang-format-14(via Homebrew'sllvm@14, matching the version CI installs) and ranmake formatuntil it was a no-op before committing - no further formatting diffs remain.Test plan
cmakeconfigure +makebuild the full project cleanly with no new warningsmake format(clang-format-14) produces no diffctestrun fortestVertex,testVertexInternal,testVertexExhaustive,testDirectedEdge,testDirectedEdgeExhaustive- all passctestrun confirms no regressions outside of pre-existing, unrelated environment-specific CLI test failures (not touched by this change)Closes #1071. Also resolves #421.