[Go] Replace references to hardcoded ”Monster" etc with idiomatic go wherever possible

[somombo]

Most details of the need for this PR were already discussed in Issue #5668.

I have also (additionally) converted the generated union types' UnPack func into a method.

e.g.
Generated func:

func AnyUnPack(t Any, table flatbuffers.Table) *AnyT

Becomes method,

func (rcv Any) UnPack(table flatbuffers.Table) *AnyT

[somombo]

Let's also deprecate the generated GetRootAsMonster func

I'm adding this deprecate suggestion to this PR because the entire goal of this PR was to remove as many of the hardcoded word "Monster" (or equivalent) from the generated file wherever possible. A lot of times when I saw an identifier with something like "Monster" in it's name, there was a better golang way to express that.. And in my opinion, the last such place where we can eliminate that sort of thing is with regards to the generated GetRootAsMonster func.

Before this PR is merged I'd also liked to propose that in addition to what's in this PR,
the local generated function GetRootAsMonster be (marked as) depricated in favor of the global flatbuffer.GetRootAs.

I think developers should always favor using global functions wherever they are available and there is no greater cost in using them. Global functions (as opposed to the generated ones) are easier to evolve with features, and will be documented in a standard predictable way (GoDocs).

As you can see in the code for the two above, their implementation is almost identical. So there's really no point in having the generated version. In the meantime, so as not to break people, I suggest that we reimplement the local version using the global version like:

// Deprecated in favor of global `flatbuffers.GetRootAs` function.
func GetRootAsMonster(buf []byte, offset flatbuffers.UOffsetT) *Monster {
    x := &Monster{}
    flatbuffers.GetRootAs(buf, offset, x)
    return x
}

/cc @aardappel

[tsingson]

LGTM, i like this flatbuffers.GetRootAs()

[iceboy233]

The GetRootAsXxx does provide some value.

I have a code base which does a lot of RPC handling with flatbuffers. Since golang does not have generics templates, I have been using the following pattern:

func handleXxx(
	env *base.Env, requestBytes []byte, responseBuilder *flatbuffers.Builder) {
	request := apifb.GetRootAsXxxRequest(requestBytes, 0).UnPack()
	response := apifb.XxxResponseFB{}
	defer func() {
		responseBuilder.Finish(apifb.XxxResponsePack(responseBuilder, &response))
	}()
	...
}

With the global flatbuffers.GetRootAs, the RPC handling stub becomes:

func handleXxx(
	env *base.Env, requestBytes []byte, responseBuilder *flatbuffers.Builder) {
	requestBuffer := apifb.XxxRequest{}
	flatbuffers.GetRootAs(requestBytes, 0, &requestBuffer)
	request := requestBuffer.UnPack()
	response := apifb.XxxResponseFB{}
	defer func() {
		responseBuilder.Finish(response.Pack(responseBuilder))
	}()
	...
}

I would have to do this for all RPC handler functions, which bloats the code a lot.

[iceboy233]

Another idea is that we can create a flatbuffers.Packable interface:

type Packable interface {
	Pack(builder *Builder) UOffsetT
}

[somombo]

Without loss of generality, I'm hereinafter referring to the local name of generated artifact as "monster"

The GetRootAsMonster does provide some value.

I have been thinking about this deeper and I have just now realized that all the GetRootAsMonster function is, is a constructor function: It's job is literally to just allocate a new *Monster and set it's initial (main) values.

As such, I now think we ought to be following the golang convention on naming for this constructor. So perhaps GetRootAsMonster should be renamed to NewMonster. This will make it abundantly clear to golang devs that this is simply serving the role of a "initializing constructor".

See the following for reference on this convention:

1. Effective Go - https://golang.org/doc/effective_go.html#composite_literals
2. Stack Question - https://stackoverflow.com/a/18125763/2959469

So perhaps the "Monster" in the the identifier for this function needn't be eliminated after all.

[somombo]

Another idea is that we can create a flatbuffers.Packable interface:

type Packable interface {
	Pack(builder *Builder) UOffsetT
}

I will go ahead and add this to this PR

[aardappel]

I guess for the moment keep both ways of getting the root, maybe mark the old as deprecated somehow?

[aardappel]

I'm generally ok with this PR, hopefully we don't break too many people.
@rw any objections before we merge?

[somombo]

Sorry,

I'd also like the generated Any's (Union) UnPack method to have the exact same signature as all other generated types.. This will allow us to create a universal interface UnPackable analogous to the Packable proposed above..

I did not achieve this in the current PR as it stands.. But just now thought of how to do this. watch this space, I'll update the PR.

[rw]

This LGTM. Thanks for keeping the PR scoped to just the Object API, it made it easier to review.

[aardappel]

Ok, thanks!

[somombo]

oh no, there still some changes to this PR I had.. I guess I'll make a new one

[rw]

@somombo We do that sometimes... best to state in big loud letters that you want to keep tweaking it. :-]

[somombo]

@rw Noted!
well atleast yay for my first PR on this repo!

watch out for go.mod next