Bump to h3 4.4.0#213
Conversation
Pull Request Test Coverage Report for Build 19382556864Details
💛 - Coveralls |
| * @static | ||
| * @param {number} res Resolution of cell to return | ||
| * @param {number} baseCellNumber Base cell number of cell to return | ||
| * @param {number[]} digits Indexing digits of cell to return | ||
| * @return {H3Index} H3 index | ||
| * @throws {H3Error} If input is invalid | ||
| */ | ||
| export function constructCell(res, baseCellNumber, digits) { | ||
| if (digits.length < res) { | ||
| throw H3LibraryError(E_DIGIT_DOMAIN, digits.length); | ||
| } | ||
| if (digits.length > 15) { | ||
| throw H3LibraryError(E_DIGIT_DOMAIN, digits.length); | ||
| } |
There was a problem hiding this comment.
Would it be natural to just have the resolution be inferred directly from the length of the digits array? I understand this matches the C function more closely, but these seems less natural for a language like JS or Python, where we would more naturally just take the length of the array.
There was a problem hiding this comment.
For example, the compactCells in JS function doesn't take in the length of the array as a separate parameter.
There was a problem hiding this comment.
I don't mind this, TBH. I think it's natural for most callers to think about the res separately from the digits being set, even though they're closely related.
There was a problem hiding this comment.
What's the difference? For this function, they should be exactly the same, no?
There was a problem hiding this comment.
discussed offline, let's move res to the end and make it optional
nrabinowitz
left a comment
There was a problem hiding this comment.
Looks good - thanks for putting this up!
| [](LICENSE) | ||
| [](https://badge.fury.io/js/h3-js) | ||
| [](https://github.com/uber/h3/releases/tag/v4.1.0) | ||
| [](https://github.com/uber/h3/releases/tag/v4.4.0) |
There was a problem hiding this comment.
Thanks, I guess we missed this in previous updates. I could potentially auto-gen this I think
| assert.end(); | ||
| }); | ||
|
|
||
| test('isValidIndex', assert => { |
There was a problem hiding this comment.
Do we want tests here to assert that an edge or a vertex is a valid index?
| }, | ||
| "dependencies": {} | ||
| "dependencies": {}, | ||
| "packageManager": "yarn@1.22.22+sha512.a6b2f7906b721bba3d67d4aff083df04dad64c399707841b7acf00f6b133b7ac24255f2652fa22ae3534329dc6180534e98d17432037ff6fd140556e2bb3137e" |
There was a problem hiding this comment.
Not sure. It's added automatically by this version of yarn. Maybe we can turn that off in the package.json settings so it doesn't propose doing that.
| } | ||
|
|
||
| /** | ||
| * Whether a given string represents a valid H3 index |
There was a problem hiding this comment.
Nit: Maybe add some clarifying text to explain the difference between isValidCell and isValidIndex
| * @static | ||
| * @param {number} res Resolution of cell to return | ||
| * @param {number} baseCellNumber Base cell number of cell to return | ||
| * @param {number[]} digits Indexing digits of cell to return | ||
| * @return {H3Index} H3 index | ||
| * @throws {H3Error} If input is invalid | ||
| */ | ||
| export function constructCell(res, baseCellNumber, digits) { | ||
| if (digits.length < res) { | ||
| throw H3LibraryError(E_DIGIT_DOMAIN, digits.length); | ||
| } | ||
| if (digits.length > 15) { | ||
| throw H3LibraryError(E_DIGIT_DOMAIN, digits.length); | ||
| } |
There was a problem hiding this comment.
I don't mind this, TBH. I think it's natural for most callers to think about the res separately from the digits being set, even though they're closely related.
No description provided.