Flatbuffers: [Go] Suggestion: Object API should generate `monster.Pack` method instead of `MonsterPack` function

[somombo]

Regarding: Golang Object API https://github.com/google/flatbuffers/pull/5339

Is it too late to suggest that things be generated in such a way that for example:

example.MonsterPack(builder, monster) would instead be used like monster.Pack(builder)

i.e. by making the MonsterPack function instead be a method of MonsterT?

I think this is more flexible

[aardappel]

Probably good to tag the authors / interested parties: @iceb0y @llchan

[somombo]

@llchan I see that in your snippet on https://github.com/google/flatbuffers/pull/5339#issuecomment-546659081,
you initially intended or expected what I'm suggesting here. Any idea why that never made it to the merged version?

[llchan]

I preferred what you are suggesting as well, but it was already implemented when I found the PR (I was mostly an observer in that process). I didn't push for it further, because I eventually discovered that this matches the C++ interface, and arguably it's better to have uniformity across languages.

I'm not opposed to changing it or adding it as an additional entry point, since that feels more ergonomic/intuitive to me. Do you have an opinion @aardappel?

[aardappel]

I'm cool with changing it, if that makes more sense for Go users.

This functionality was only recently added, and is not in any releases yet, so I don't expect to break a lot of people.

[somombo]

Here are my exact proposed changes

For @llchan's Marshall in #5593

Refer to: idl_gen_go.cpp GenNativeStructPack Line814

Change

    code += "\tb.Finish(" + struct_def.name + "Pack(b, t))\n";

to:

    code += "\tb.Finish(t.Pack(b))\n";

For Unions, Tables and Structs

Refer to idl_gen_go.cpp# GenNativeUnionPack, GenNativeTablePack, and GenNativeStructPack

Change (in three places) something like:

    code += "func " + struct_def.name +
            "Pack(builder *flatbuffers.Builder, t *" + NativeName(struct_def) +
            ") flatbuffers.UOffsetT {\n";

To:

    code += "func (t *" + NativeName(struct_def) +
            ") Pack(builder *flatbuffers.Builder) flatbuffers.UOffsetT {\n";

@aardappel if there is nothing else more complex to it, I can create PR. What do you think?

[aardappel]

@somombo yup that looks good to me, go ahead :)

[somombo]

Great!
@llchan how do we do this? Do I wait on #5593 to be merged?

[tsingson]

func (m *MonsterT) Pack(b *flatbuffers.Builder) flatbuffers.UOffsetT {
  ...
}

func (m *MonsterT) Pack(b *flatbuffers.Builder) flatbuffers.UOffsetT ------- it's perfect for go.

but func MonsterPark need to keep and mark as "Deprecated" , like this:

// DEPRECATED: Use monster.Park instead.
func MonsterPack(builder *flatbuffers.Builder, t *MonsterT) flatbuffers.UOffsetT {
      if t == nil {
     return 0
     }
      return t.Park(builder)
}

for this:

func (m *MonsterT) Unmarshal(data []byte) error {
  GetRootAsMonster(data, 0).UnpackTo(m)
  return nil
}

this Unmarshal func, be panic in go when *MonsterT is nil pointer.

i add func UnmarshalMonsterT( b []byte) *MonsterT in https://github.com/google/flatbuffers/pull/5705 , to keep old style function name , it's work in my business trial project

for all go code, maybe need more error handle like nil pointer check ........

@llchan @somombo

[somombo]

https://github.com/google/flatbuffers/issues/5668#issuecomment-567715947
This functionality was only recently added, and is not in any releases yet, so I don't expect to break a lot of people.

@tsingson I dont think we should maintain backward compatibility on features that havent been incorporated in an official release yet. Better to make a clean break early than to bloat the generated code.

[llchan]

@somombo feel free to make this change first, I can rebase that other PR. It sits at a higher level than this stuff so it should probably be merged after this.

[tsingson]

@somombo agree .

[tsingson]

wait for https://github.com/google/flatbuffers/pull/5593 ?

the https://github.com/google/flatbuffers/pull/5593 is early PR like i do.

1. in https://github.com/google/flatbuffers/pull/5593, * Unmarshal()* func need more attention, it's unsafe now.....
2. --go-object-marshal , i don't think it's good idea: for flatc, adding special parameter options to the Go language is unnecessary.

[llchan]

@tsingson lets keep the #5593 comments on that PR so that this issue can stay focused on the Pack functions

[aardappel]

Ah sorry didn't see this discussion.

Yes, ok to make breaking changes on very recent features.

[somombo]

I have posted pr https://github.com/google/flatbuffers/pull/5716
addressing this and other issues

[somombo]

closed #5716