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

drop Cells from function names when implicit #451

Closed
ajfriend opened this issue Apr 20, 2021 · 5 comments
Closed

drop Cells from function names when implicit #451

ajfriend opened this issue Apr 20, 2021 · 5 comments

Comments

@ajfriend
Copy link
Contributor

The conversation for #445 and #441 were a bit scattered and seem to have died, so I'm creating a fresh issue to discuss.

En Garde

The gist of the change is at #441 (comment), reproduced here:

We might consider this new "cell is assumed if omitted" rule for a few other functions:

Current library name Current RFC proposal
compact compactCells
uncompact uncompactCells
polyfill polygonToCells

Of those, I think polygonToCells is an improvement, but the compact/uncompact functions probably don't need the full specificity of "Cells", which can just be implicit. What would folks think about the following change to the RFC?

Current library name New RFC proposal
compact (unchanged)
uncompact (unchanged)
polyfill polygonToCells

Parry

@dfellis raises a valid concern that dropping Cells from some function names (and thus making them implicit), could be making the library less consistent:

Riposte

I argue that the change maintains consistency as long as we agree that an implicit Cells is OK for functions where we think users can reasonably recognize the implication.

polyfill vs polygonTo<X> remains an open question, however.

Reprise

(to follow)

@dfellis
Copy link
Collaborator

dfellis commented Apr 20, 2021

Well, since I'm still awake and waiting on a test build, I guess I can respond now!

I don't think the ToCells/Cells at the end of the function is that bad, and it keeps the clarity very high on what you're gonna get out of the function.

One thing that the regularity of the function naming buys us besides that slight increase in clarity, is that the bindings in the object-oriented languages (basically all of them) could provide an object-like API in their languages that fits better with how things work in that language, and this has been further unblocked by the new website that has language-specific docs.

Imagine something like:

from h3 import Cell

c = Cell('822d57fffffffff') # Returns a `Cell`
c.to_lat_lng() # Returns a `LatLng` that could be turned into a simple `[lat, lng]` or `[lng, lat]` pair
p = c.to_polygon() # Returns the outline of the cell as a `Polygon`
cs = p.to_cells() # Returns a set of cells that contains the polygon, which *should* be the original cell only
ccs = cs.compact() # Would return the same set
ucs = ccs.uncompact() # Would also return the same set

The fooToBar pattern in the C function naming pattern could be used to automatically generate this kind of API, and even the fooBar pattern could be detected (eg compactCells is a function with a Cells suffix so it should be parsed as a compact method on the Cells object).

All three of the "main" bindings would be amenable to this, while the object-less C would have the functions with a regular naming so those familiar with this form should adapt decently well to working with it. Violating this regularity would make the mapping to objects in OO languages much more arbitrary and up for even more debate, which we have too much of as it is. ;)

@nrabinowitz
Copy link
Collaborator

I don't have a strong opinion here, but one argument in favor of toCells is that there's no type safety (at least in C) around which type of H3 index a function operates on. I don't believe that compact or uncompact will work correctly with edges or vertexes, but I can easily imagine a compactVertexes or compactDirectedEdges function that would work correctly. If we wanted these functions in the future, it would be more confusing to add it if there was a bare compact already in the API.

@ajfriend
Copy link
Contributor Author

OK, so I forgot to add the example that actually kicked off this whole line of thought. It was when @nrabinowitz was pointing out that the name gridPathCells wasn't a great name, and I think we came to the conclusion that gridPath would be a better name (with Cells) being implicit: #403 (comment)

The following issues/PRs were me trying to come up with some consistent system that allowed for gridPath, which is what got us to the whole "implicit Cell" business. :)

@nrabinowitz, would you say that you now prefer gridPathCells to gridPath?

Also, @isaacbrodsky, you seemed to be in support of the proposal in #441, which is why I landed it at that point: #441 (review) I'm curious to hear your thoughts at this point?

@ajfriend
Copy link
Contributor Author

And for the record, I am won over to the side of keeping Cells explicitly in the function names, as long as gridPathCells doesn't bother you too much @nrabinowitz :)

@ajfriend
Copy link
Contributor Author

Chatted about this offline. I've come around to agreeing that keeping Cells explicit would be good for the C library. (That may or may not also hold for the bindings, but we can figure that out as it comes up).

I'll close this and revert #441 .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants