Skip to content

Add constructCell to CLI#1078

Merged
isaacbrodsky merged 8 commits intouber:masterfrom
LucaDiba:constructCell-cli
Nov 6, 2025
Merged

Add constructCell to CLI#1078
isaacbrodsky merged 8 commits intouber:masterfrom
LucaDiba:constructCell-cli

Conversation

@LucaDiba
Copy link
Copy Markdown
Member

@LucaDiba LucaDiba commented Nov 1, 2025

  • Add constructCell to CLI.
  • Add tests.
  • Update docs.

Comment thread src/apps/filters/h3.c Outdated
@coveralls
Copy link
Copy Markdown

coveralls commented Nov 1, 2025

Coverage Status

coverage: 98.946%. remained the same
when pulling 7edff86 on LucaDiba:constructCell-cli
into 1de1552 on uber:master.

@ajfriend ajfriend requested a review from dfellis November 1, 2025 15:10
Comment thread src/apps/filters/h3.c Outdated
@ajfriend ajfriend added this to the v4.4 milestone Nov 1, 2025
@ajfriend
Copy link
Copy Markdown
Collaborator

ajfriend commented Nov 1, 2025

This is great. Thanks @LucaDiba!

Would you also be able to add a short description for the CLI in the docs (which I skipped in #1077, because I wasn't sure what the final API would look like)?

@LucaDiba
Copy link
Copy Markdown
Member Author

LucaDiba commented Nov 1, 2025

Would you also be able to add a short description for the CLI in the docs (which I skipped in #1077, because I wasn't sure what the final API would look like)?

Yup, updated.

Comment thread src/apps/filters/h3.c Outdated
@ajfriend
Copy link
Copy Markdown
Collaborator

ajfriend commented Nov 2, 2025

Here's a thought: What about making the resolution parameter optional? We can infer the res from the length of the digits.

I'm raising it here because I'm considering doing something like that for the Python bindings.

Another solution would be to not have a resolution parameter at all, and just read from the length of the digits. That's not so different from what, say, compactCells does in the CLI; we don't pass in the number of cells. The res parameter is redundant if you have another way to know the length of the list.

@dfellis
Copy link
Copy Markdown
Collaborator

dfellis commented Nov 2, 2025

Here's a thought: What about making the resolution parameter optional? We can infer the res from the length of the digits.

I'm raising it here because I'm considering doing something like that for the Python bindings.

Another solution would be to not have a resolution parameter at all, and just read from the length of the digits. That's not so different from what, say, compactCells does in the CLI; we don't pass in the number of cells. The res parameter is redundant if you have another way to know the length of the list.

I would be fine with optional, as long as an explicit mismatch results in an error. It can be useful to have it automatically inferred when using it manually, but including the resolution inside of a batch scripting job so that you can find corrupt input data.

Comment thread src/apps/filters/h3.c Outdated
@LucaDiba
Copy link
Copy Markdown
Member Author

LucaDiba commented Nov 6, 2025

Here's a thought: What about making the resolution parameter optional? We can infer the res from the length of the digits.

I would be fine with optional, as long as an explicit mismatch results in an error. It can be useful to have it automatically inferred when using it manually, but including the resolution inside of a batch scripting job so that you can find corrupt input data.

Makes sense, I like this approach. Updated.

Copy link
Copy Markdown
Collaborator

@ajfriend ajfriend left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great. Thanks!

@isaacbrodsky isaacbrodsky merged commit ccde674 into uber:master Nov 6, 2025
45 checks passed
@LucaDiba LucaDiba deleted the constructCell-cli branch November 6, 2025 18:21
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.

5 participants