Skip to content

Conversation

@LucaDiba
Copy link
Contributor

@LucaDiba LucaDiba commented Nov 1, 2025

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

@coveralls
Copy link

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
@ajfriend ajfriend added this to the v4.4 milestone Nov 1, 2025
@ajfriend
Copy link
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
Contributor 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.

@ajfriend
Copy link
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
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.

@LucaDiba
Copy link
Contributor 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
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