Add H3Error for core indexing functions - #436
Merged
Merged
Conversation
dfellis
approved these changes
Mar 16, 2021
dfellis
left a comment
Collaborator
There was a problem hiding this comment.
Nonblocking comments. Can always come up in a followup PR.
| if (e == E_SUCCESS) { | ||
| h3Println(h); | ||
| } else { | ||
| h3Println(H3_NULL); |
Collaborator
There was a problem hiding this comment.
Ideally it would print some nice data about which error it ran into to stderr instead. :)
Comment on lines
+74
to
+96
| typedef enum { | ||
| E_SUCCESS = 0, // Success (no error) | ||
| E_FAILED = | ||
| 1, // The operation failed but a more specific error is not available | ||
| E_DOMAIN = 2, // Argument was outside of acceptable range (when a more | ||
| // specific error code is not available) | ||
| E_LATLON_DOMAIN = | ||
| 3, // Latitude or longitude arguments were outside of acceptable range | ||
| E_RES_DOMAIN = 4, // Resolution argument was outside of acceptable range | ||
| E_CELL_INVALID = 5, // `H3Index` cell argument was not valid | ||
| E_DIR_EDGE_INVALID = 6, // `H3Index` directed edge argument was not valid | ||
| E_UNDIR_EDGE_INVALID = | ||
| 7, // `H3Index` undirected edge argument was not valid | ||
| E_VERTEX_INVALID = 8, // `H3Index` vertex argument was not valid | ||
| E_PENTAGON = 9, // Pentagon distortion was encountered which the algorithm | ||
| // could not handle it | ||
| E_DUPLICATE_INPUT = 10, // Duplicate input was encountered in the arguments | ||
| // and the algorithm could not handle it | ||
| E_NOT_NEIGHBORS = 11, // `H3Index` cell arguments were not neighbors | ||
| E_RES_MISMATCH = | ||
| 12, // `H3Index` cell arguments had incompatible resolutions | ||
| E_MEMORY = 13 // Necessary memory allocation failed | ||
| } H3ErrorCodes; |
Collaborator
There was a problem hiding this comment.
Would be really nice if these comments were available in an exported array of strings (and/or a function to convert error code to error string). Could be used by the bindings as well as the filter apps.
Collaborator
There was a problem hiding this comment.
Agree, we duplicate a lot of this in JS
nrabinowitz
approved these changes
Mar 25, 2021
|
|
||
| ``` | ||
| void h3ToGeoBoundary(H3Index h3, GeoBoundary *gp); | ||
| H3Error h3ToGeoBoundary(H3Index h3, GeoBoundary *gp); |
Collaborator
There was a problem hiding this comment.
Nit: This should be gb?
| printf(" "); | ||
| geoPrintlnNoFmt(&g); | ||
| H3Index h; | ||
| if (!H3_EXPORT(geoToH3)(&g, res, &h)) { |
Collaborator
There was a problem hiding this comment.
Should we use != E_SUCCESS in this and similar cases?
Comment on lines
+74
to
+96
| typedef enum { | ||
| E_SUCCESS = 0, // Success (no error) | ||
| E_FAILED = | ||
| 1, // The operation failed but a more specific error is not available | ||
| E_DOMAIN = 2, // Argument was outside of acceptable range (when a more | ||
| // specific error code is not available) | ||
| E_LATLON_DOMAIN = | ||
| 3, // Latitude or longitude arguments were outside of acceptable range | ||
| E_RES_DOMAIN = 4, // Resolution argument was outside of acceptable range | ||
| E_CELL_INVALID = 5, // `H3Index` cell argument was not valid | ||
| E_DIR_EDGE_INVALID = 6, // `H3Index` directed edge argument was not valid | ||
| E_UNDIR_EDGE_INVALID = | ||
| 7, // `H3Index` undirected edge argument was not valid | ||
| E_VERTEX_INVALID = 8, // `H3Index` vertex argument was not valid | ||
| E_PENTAGON = 9, // Pentagon distortion was encountered which the algorithm | ||
| // could not handle it | ||
| E_DUPLICATE_INPUT = 10, // Duplicate input was encountered in the arguments | ||
| // and the algorithm could not handle it | ||
| E_NOT_NEIGHBORS = 11, // `H3Index` cell arguments were not neighbors | ||
| E_RES_MISMATCH = | ||
| 12, // `H3Index` cell arguments had incompatible resolutions | ||
| E_MEMORY = 13 // Necessary memory allocation failed | ||
| } H3ErrorCodes; |
Collaborator
There was a problem hiding this comment.
Agree, we duplicate a lot of this in JS
isaacbrodsky
force-pushed
the
error-returns-h3togeo
branch
from
April 8, 2021 00:35
2c87037 to
70d4b6a
Compare
dfellis
reviewed
Apr 8, 2021
Comment on lines
+33
to
+34
| SUITE(h3UniEdge) { | ||
| TEST(h3IndexesAreNeighbors) { |
dfellis
approved these changes
Apr 8, 2021
dfellis
left a comment
Collaborator
There was a problem hiding this comment.
Just one accidental revert when you rebased that needs to be cleaned up, as far as I can see.
Collaborator
Author
|
I updated this PR based on master, note that I removed the website changes until we're ready to make a v4 website |
ajfriend
approved these changes
Apr 9, 2021
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
First implementation of the error code RFC work for v4. In this PR the error codes are applied to only
geoToH3,h3ToGeo, andh3ToGeoBoundaryin order to get feedback on these core functions first.H3Error is not made an enum type since from what I could determine setting the base type of the enum is a C++ feature. (Although when I test using Apple Clang it permits it even with
-std=c89?)