Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor: Hex location use hextree::Cell type #797

Merged
merged 2 commits into from
Apr 26, 2024

Conversation

michaeldjeffrey
Copy link
Contributor

This is something that I was planning on doing as part of HIP-109 changes I was going to be making.
So I thought I'd push it early to make it easier to review.


Got some advice from Jay to only use a h3o::CellIndex when you need to
convert to/from a LatLng. So we went with hextree::Cell for the type.

hextree::Cell is a loose wrapper around a u64. Parsing the value as
early as possible tightens the gaurantee that we can make valid types
that need use Cells.

It is also an attempt to remove discrepancies where some types carry a
i64 hex straight from a database, and other types carry a u64 that
were converted out of a database, even though they're goal is the same.

(see previous commit where #[sqlx(try_from = "i64")] was enabled by
the update to hextree).

Copy link
Contributor

@andymck andymck left a comment

Choose a reason for hiding this comment

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

I think this is fine in itself and it is better to have a consistent representation of a location. The only point I would make is that maybe it doesnt go wide enough. We now have a location for the purposes of boosted hexes defined as a Cell but in all other uses of location we still have it as a u64. This inconsistency niggles me. Most non boosted hex sources of location data originates from mobile config's gateway info, so perhaps that should be changed to a Cell and span out from there.

mobile_config/src/boosted_hex_info.rs Outdated Show resolved Hide resolved
@michaeldjeffrey michaeldjeffrey force-pushed the mj/hex-location-use-cell-type branch from f276eff to e3e5057 Compare April 23, 2024 15:54
@michaeldjeffrey
Copy link
Contributor Author

The only point I would make is that maybe it doesnt go wide enough. We now have a location for the purposes of boosted hexes defined as a Cell but in all other uses of location we still have it as a u64. This inconsistency niggles me.

That's where I'd like to be. Considering all this change fanned out from the BoostedHex::location field, it seemed more responsible to switch over only the things related to what I plan on touching and get that change in. Rather than starting from the source and me having to make decisions for parts of the code I haven't interacted with yet, and landing a behemoth of a refactor PR.

Update includes an implementation of TryFrom<i64> for Cell.

This allows adding `#[sqlx(try_from = "i64")]` to struct fields of `hextree::Cell`.

JayKickliter/HexTree#45
Got some advice from Jay to only use a `h3o::CellIndex` when you need to
convert to/from a LatLng. So we went with `hextree::Cell` for the type.

`hextree::Cell` is a loose wrapper around a u64. Parsing the value as
early as possible tightens the gaurantee that we can make valid types
that need use `Cell`s.

It is also an attempt to remove discrepancies where some types carry a
`i64` hex straight from a database, and other types carry a `u64` that
were converted out of a database, even though they're goal is the same.

(see previous commit where `#[sqlx(try_from = "i64")]` was enabled by
the update to hextree).
@michaeldjeffrey michaeldjeffrey force-pushed the mj/hex-location-use-cell-type branch from e3e5057 to e6789ce Compare April 26, 2024 01:34
@michaeldjeffrey michaeldjeffrey merged commit 9c78cfa into main Apr 26, 2024
1 check passed
@michaeldjeffrey michaeldjeffrey deleted the mj/hex-location-use-cell-type branch April 26, 2024 19:32
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.

3 participants