Add quadbinToBoundary utility #16
Conversation
Co-authored-by: felixpalmer <[email protected]>
src/index.ts
Outdated
| @@ -13,6 +15,29 @@ const S = [0n, 1n, 2n, 4n, 8n, 16n]; | |||
| type Quadbin = bigint; | |||
| type Tile = {x: number; y: number; z: number}; | |||
|
|
|||
| const TILE_SIZE = 512; | |||
|
|
|||
| function quadbinToOffset(quadbin: bigint): [number, number, number] { | |||
There was a problem hiding this comment.
Could you rename these 3 functions to be cellTo... and export them?
There was a problem hiding this comment.
I think you missed this comment
There was a problem hiding this comment.
I kept them as they are because they have this name in deck.gl, and not only that, but they receive a parameter quadbin, so I think we should re-think it a bit before renaming?
Regarding your export suggestion, done!
There was a problem hiding this comment.
I think if we are going to add functions to the package we shouldn't change the names later. So best to include them as cellToOffset(cell: bigint) etc, i.e. renaming also the parameter. It's not an issue for deck that the name changes
There was a problem hiding this comment.
What I'm afraid of is that we are proposing some changes that may require a major version, and I'm not sure if we should go for them in this PR, just it 🤷
What's your proposal then? Mine is:
- Stick to
Quadbintype everywhere - Use
Quadbininstead ofcellin all the function names - Release a major
There was a problem hiding this comment.
As stated originally, I want the API to be aligned with quadbin-py. Which it currently is.
- Any new functions which are in
quadbin-pyshould have the same name - Any additional functions, like
cellToOffsetshould follow the conventions already used
I see no value in renaming the existing function names, that would be a breaking change and require a major version and diverge from quadbin-py.
For the new functions we should stick with the conventions, so using cell in function names and quadbin: Quadbin in the parameters. It doesn't matter to the calling code what the name of the parameter is
There was a problem hiding this comment.
The API is modeled on https://github.com/uber/h3-js, which means that it is simpler to port code between the spatial indices, which is not the case if the functions are things like quadbinToBoundary
There was a problem hiding this comment.
For the new functions we should stick with the conventions, so using cell in function names and quadbin: Quadbin in the parameters. It doesn't matter to the calling code what the name of the parameter is
Sounds like a plan, let's go for it 👍
There was a problem hiding this comment.
I'll say now, every function:
- Receives a
quadbin: Quadbin - Uses
cellin its name
Could you please re-re-re-review it 😆 ?
The main goal of this Pull-Request is to add support for an utility we require here
I also did some refinements, adding a
CONTRIBUTING.mdfile, along with some fixes in thepackage.jsonthat will prevent different builds ifnpxresolves some dependencies differently 👍