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

Rename experimentalLocalIjToH3 to localIjToCell #586

Merged
merged 8 commits into from
Mar 15, 2022

Conversation

isaacbrodsky
Copy link
Collaborator

@isaacbrodsky isaacbrodsky commented Feb 28, 2022

Renames experimental... functions to no longer be experimental, as they have been in the library for some time now. Also renames the functions to be in line with the naming RFC.

@coveralls
Copy link

coveralls commented Feb 28, 2022

Coverage Status

Coverage increased (+0.003%) to 98.657% when pulling b454116 on isaacbrodsky:isaac/cell-to-local-ij into 30d6464 on uber:master.

@dfellis
Copy link
Collaborator

dfellis commented Feb 28, 2022

This does not include the mode that was discussed? Part of dropping the experimental part of the name was that with a mode we could add improved versions without breaking existing users.

@isaacbrodsky
Copy link
Collaborator Author

This does not include the mode that was discussed? Part of dropping the experimental part of the name was that with a mode we could add improved versions without breaking existing users.

Correct - I can certainly add that in this PR if you think it makes sense with the renaming.

@dfellis
Copy link
Collaborator

dfellis commented Mar 1, 2022

Correct - I can certainly add that in this PR if you think it makes sense with the renaming.

Yeah, I think they go together.

@isaacbrodsky
Copy link
Collaborator Author

Yeah, I think they go together.

Updated

@@ -519,10 +519,15 @@ H3Error localIjkToCell(H3Index origin, const CoordIJK *ijk, H3Index *out) {
*
* @param origin An anchoring index for the ij coordinate system.
* @param index Index to find the coordinates of
* @param flags Mode flags, must be 0
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we want this to be a set of bitflags or an integer mode? I thought we went for the latter because we couldn't think of any combinatorial flags that would make any sense?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm treating it as an opaque mode identifier without structure until we decide further.

Copy link
Collaborator

Choose a reason for hiding this comment

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

So, I meant the docs all say "flags"? And I thought we had decided to just make it a mode enum/integer?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I see what you mean. In my mind these were interchangeable (I could just say there's a single flags bit field of length 32 bits). I standardized on mode.

H3Error H3_EXPORT(localIjToCell)(H3Index origin, const CoordIJ *ij,
uint32_t mode, H3Index *out) {
if (mode != 0) {
return E_FAILED;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggestion: make a special E_INVALID_MODE? I imagine we'll need this for other cases too, and for polygonToCells

@isaacbrodsky isaacbrodsky merged commit ba89a19 into uber:master Mar 15, 2022
@isaacbrodsky isaacbrodsky deleted the isaac/cell-to-local-ij branch March 15, 2022 16:37
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.

4 participants