Conversation
Similar to uber#1042
| H3_EXPORT(isValidDirectedEdge)(cell) || | ||
| H3_EXPORT(isValidVertex)(cell); | ||
| if (!isValid) { | ||
| return E_DOMAIN; // TODO: maybe create a new E_INDEX_INVALID error? |
There was a problem hiding this comment.
Looking at the set of errors I would probably just go with E_FAILED. You can see in the new tests that the error message given when these sub-commands are called incorrectly is confusing with the current text it returns.
Looking at the text it can select from that would still be sub-optimal and E_CELL_INVALID's error message would be more legible, but it also wouldn't be correct in some situations, so I'm not so sure about it.
There was a problem hiding this comment.
Should we follow up with a method that wraps this logic for us (3x valid checks) and add a generic index invalid error?
There was a problem hiding this comment.
I would like some feedback from @isaacbrodsky @nrabinowitz and @ajfriend but I do think a general isValid function and a new error code would be better.
There was a problem hiding this comment.
I think it would make sense to add an E_INVALID_INDEX (#1044) and associated helper function. I am also fine with this PR as I understand it corrects the check that the index is valid.
There was a problem hiding this comment.
I'd be in favor of #1043 and #1044. Up to you, @justinhwang if you want to do that now, or if we should follow up on it.
Although, we'd should probably make those changes before the next release to minimize the number of times we modify the error codes.
Similar to #1042