Skip to content

Fix gridRing CLI printing 0 lines in newline output near pentagons - #1277

Open
Feldt wants to merge 1 commit into
uber:masterfrom
Feldt:fix/cli-gridring-newline-zeros
Open

Feldt wants to merge 1 commit into
uber:masterfrom
Feldt:fix/cli-gridring-newline-zeros

Conversation

@Feldt

@Feldt Feldt commented Oct 20, 2026

Copy link
Copy Markdown

h3 gridRing -f newline printed a 0 line for each unused slot of its output array. The CLI sizes that array with maxGridRingSize (6 * k), and near a pentagon the ring has fewer cells, so the library leaves the remaining slots as 0 (the gridRing docs say elements may be left zero). The json branch of the same subcommand already skips zero slots, and so does the newline branch of gridDisk. The newline branch of gridRing did not.

On master:

$ h3 gridRing -c 820807fffffffff -k 1 -f newline
820817fffffffff
82081ffffffffff
820827fffffffff
82082ffffffffff
820837fffffffff
0

The json output of the same call lists only the 5 neighbours of the pentagon. Hexagons near a pentagon hit this too: -c 820817fffffffff -k 2 -f newline prints 11 cells and a 0.

Changes:

  • src/apps/filters/h3.c: the gridRing newline branch skips zero entries, the same check gridDisk uses.
  • tests/cli/gridRing.txt: new testCliGridRingPentagonNewline, which counts the newline output lines for a pentagon at k = 1 and expects 5. It fails on master (6 lines) and passes with the fix. It counts lines rather than comparing the cell list, because the fallback ring's cell order comes from the gridDisk internals, and Reimplement gridDisk internals with BFS #1217 changes that order.
  • CHANGELOG.md: a line under Unreleased / Fixed.

Test:

cmake -Bbuild -DBUILD_TESTING=ON .
cd build && make && ctest -R testCliGridRing --output-on-failure

With the fix, the full ctest run passes in Debug and Release builds with -DWARNINGS_AS_ERRORS=ON on macOS, and make format with clang-format 14 leaves no diff.

I prepared this change with an AI coding agent (Claude Code). I'm responsible for it and will answer review comments.

🤖 Generated with Claude Code

The gridRing subcommand sizes its output array with maxGridRingSize
(6 * k), but near a pentagon the ring has fewer cells and the library
leaves the unused slots as 0. The json branch already skipped those
slots; the newline branch printed every slot, so for example
`h3 gridRing -c 820807fffffffff -k 1 -f newline` printed the pentagon's
5 neighbours followed by a line containing `0`. The newline branch now
skips zero entries, as gridDisk's newline branch does.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Oct 20, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: d263cac3-a1d1-4d39-baa4-dff51e7d84ba
📥 Commits

Reviewing files that changed from the base of the PR and between 28d83cd and 3db8eab.

📒 Files selected for processing: 3
  • CHANGELOG.md
  • src/apps/filters/h3.c
  • tests/cli/gridRing.txt

Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 7 remain after this review.

Walkthrough

The gridRing CLI now skips zero-valued entries in newline output. A CLI test checks that a pentagon cell with -k 1 produces five lines. The Unreleased changelog records the change.

Changes

gridRing newline output

Layer / File(s) Summary
Filter output and regression test
src/apps/filters/h3.c, tests/cli/gridRing.txt, CHANGELOG.md
The newline formatter skips zero-valued entries. A CLI test checks pentagon output with -k 1 and expects five lines. The changelog records the fix.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~8 minutes

Change: Bug fix

Suggested reviewers: dfellis

Merge Risk: ⚪ Minimal · up to 3db8e

No merge-blocking risk was identified in this narrowly scoped output-format fix.

Pre-merge checks: Passed 5
Check name Status Explanation
Description check Passed The description clearly explains the pentagon-related gridRing newline bug, the code and test changes, and the validation performed.
Title check Passed The title clearly and concisely identifies the fix for incorrect gridRing CLI newline output near pentagons.
Docstring Coverage Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 1…
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage

Explanation

No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 1 files. (2 skipped: 2 unsupported.)

✨ 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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@CLAassistant

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@coveralls

Copy link
Copy Markdown

Coverage Status

coverage: 99.758%. remained the same — Feldt:fix/cli-gridring-newline-zeros into uber:master

@isaacbrodsky

Copy link
Copy Markdown
Collaborator

@Feldt could you sign the CLA so we can merge your change?

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants