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

Field name restrictions tied too closely to Go language rules #206

Closed
rvagg opened this issue Jul 22, 2021 · 3 comments
Closed

Field name restrictions tied too closely to Go language rules #206

rvagg opened this issue Jul 22, 2021 · 3 comments

Comments

@rvagg
Copy link
Member

rvagg commented Jul 22, 2021

Will did over the go-ipld-git codec with ipld-prime, but it changed the serialization from lower-case git-standard names (tree, object, date etc.) to CamelCase names (Tree, Object, Date, etc.). Which seems reasonable, but it messed up the pathing resolution upstream, failing a go-ipfs test that was reaching down deep through a git DAG. Instead of brute-forcing a change to the path, I wanted to try and reconcile it with the original (and with the JS codec, and with Git standard naming—although this point is arguably flexible): https://github.com/ipfs/go-ipld-git/blob/0436bc7c4a2712d018bd8b49833745e1f7e12227/gen/gen.go

A couple of things arise from this:

  • The lower-case field names mean that outside of the package you couldn't use the plain struct forms to construct your objects (_Commit{...}), and I suppose you can't interrogate them that way either. Presumably you'd have to assemble the node using the standard assemblers and assign to the typed form. Which gets very verbose.
  • You can't use the field name type, so there's a substitution here for tagType. Which I guess means this is a reserved word in the IPLD data model in certain places.

Perhaps there's more than this. I'm wondering if we shouldn't be doing a bit of name mangling to make more cases work, like prefixing all fields with Field_ or somesuch. Or, maybe simply recognising these cases where the names will cause problems and mangling only those, predictably.

@mvdan
Copy link
Contributor

mvdan commented Jul 22, 2021

This feedback is directed at ipld-prime's codegen, right? Because I don't think the schema language itself brings any of these problems.

I've actually been thinking of similar problems in bindnode, too. The package supports inferring an IPLD schema from a Go type and vice versa, so it has some defaults to guess what a reasonable name translation would be, but I want to add support for "overrides" as well. Or, if you bring both a schema and a Go type, you can use whatever names you want, as long as the "shape" of the types matches - e.g. a struct is composed of the same field types in order.

@warpfork
Copy link
Collaborator

I believe you should be able to escape this problem in the codegen tool as well by using some of the overrides in the AdjunctCfg struct, such as these fields:

FieldSymbolLowerOverrides map[FieldTuple]string

(It may remain somewhat hacky and/or at worst require some minor patching to make the fields accessible, though.)

@willscott willscott changed the title Field name restrictions tied to closely to Go language rules Field name restrictions tied too closely to Go language rules Jul 30, 2021
@rvagg
Copy link
Member Author

rvagg commented Aug 2, 2021

Did this in ipfs/go-ipld-git#46 and it's 👌. Might need more documentation but at least there's a path here.

I think I'm not unhappy with the state of things now that I've managed to make that codec work. There is a pattern used heavily there of instantiating and modifying the structs and their fields directly rather than through the node interface methods which won't be available outside of the package. But I'm unsure if this kind of access if even intended:

c.parents.x = append(c.parents.x, _Commit_Link{cidlink.Link{Cid: shaToCid(psha)}})

Having these internal fields named things like x and v would seem to suggest they're for internal use only anyway and either shouldn't be used like this or their use should be reserved for the packages where the codegen lives (x and v can't be read outside of those packages anyway).

@rvagg rvagg closed this as completed Aug 2, 2021
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

No branches or pull requests

3 participants