-
Notifications
You must be signed in to change notification settings - Fork 50
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
The naming of Node's single step "traversal" methods #22
Comments
By the same logic, |
Is it really necessary to have Can you explain Re "traverse", it's the word I've been using for my single-node traversal algorithms, it makes sense to me. Although I do tend to have things called "traversals" that I operate on so that might be a difference for you - a traversal is made up of individual steps, each executed separately. Unless you're interested on coming up with a concrete |
The main use of it is that Selectors return If we didn't have a
Imagine you have some types like:
If someone uses Instead, by handing your If working with codegenerated implementations, then |
re: the naming prefix overall: I wonder if just plain Terse is good. It's a common operation. 'Get' is familiar and common in many communities when describing handling maps. This seems to look okay:
|
Alternatives to
|
👍 it's reminiscent of a standard At the moment |
Indeed. TODO items. |
I'll probably act on this soon -- in the next couple of days. These are pretty foundational shakeups, and although fortunately they'll all be "sed refactors" in complexity, they nonetheless show up everywhere, so it's desirable to get them done before there's too many users. In particular, it will be really good to get these done before graphsync and its transitive users start using these interfaces heavily... however, (ironically?) I'm waiting for some of the other Selectors branches to land before going ahead one way or the other. |
(I started actually trying these, and when I started seeing examples... Fully worked:
There's one more awkward bit in this set, though. |
Okay, one more take:
This solves the weird vibe by replacing And then it also solves the codegen'd name question by taking it a different direction. Notice the return tuple also is different for that one: it doesn't need to return a distinct error value. There's no need for errors like Retracted I think I'm actually completely satisfied with this one. (Some of the "By" phrasing might still show up in other places -- e.g. Thanks to @DMarby for some conversation outside of the githubs which helped refine this! |
Most important things first! To follow this refactor: ``` sed s/TraverseField/LookupString/g sed s/TraverseIndex/LookupIndex/g ``` It is *literally* a sed-refactor in complexity. --- Now, details. This has been pending for a while, and there is some discussion in #22 . In short, "Traversal" seemed like a mouthful; "Field" was always a misnomer here; and we've discovered several other methods that we *should* have in the area as well, which necessitated a thought about placement. In this commit, only the renames are applied, but you can also see the outlines of two new methods in the Node interface, as comments. These will be added in future commits. Signed-off-by: Eric Myhre <[email protected]>
(... much later ...) One more reneg on this has now occurred. 🙈 Eliding the "By" from the name turned out to be confusing, and I now regard that as having been a mistake. Also, using "Get" for codegen and "Lookup" for everything else turned out to be confusing. Both mistakes are being rectified. The final result is:
These will become the exported interfaces in (Happily, all the changes are just renames. Semantics stay the same!) |
The
ipld.Node
interface currently has a methodTraverseField(key string) (ipld.Node, error)
. This has been sufficient to get going, but there are several issues with this: it's verbose; it always takes a bare string (which is sometimes not going to be ideal); and the name "field" is usually only used in the context of structs, but we're also using it here in describing maps, which is thus awkward. Should we rename this? Furthermore, should we have several methods here?It would seem we want all of these methods at some point, and since Go eschews parametric polymorphism, they all have to have distinct names:
TraverseByString(fieldname string)
-- for use with mapsTraverseByIndex(idx int)
-- for use with listsTraverseBySegment(segment IntOrString)
-- for use when you've got pathsegments (selectors do this rather than the above two)TraverseByNode(node Node)
-- for generic use that's not segment-powered (retainstyped.Node
reference if applicable: preferable if you've got a struct-represented-as-string as a map key, or anything that has aValidate()
method on it that would need to be reapplied if we forgot about associated type info).Traverse(x ConcreteGeneratedType)
-- for use when codegen is in flight (similar advantages to TraverseByNode).If that above fivesome goes through, the first four would on the Node interface now (contrast currently only two) (the fifth is only for codegen).
Questions:
I've been thinking about this (but pushing it off) for a while now, but if something is to be done here, it should probably be a clean break soon, before we start getting too many additional things depending on this API.
The text was updated successfully, but these errors were encountered: