`json+struct` codec should provide stuff in the public API that is needed by clients

[bhaller]

Making a new issue for this at the request of @jeromekelleher since #3306 was merged and closed. Tagging @petrelharp since he'll be a client of this API too, in pyslim.

Copy-paste from #3306:

The stuff in test_core.c that puts together an example for testing would be useful to have in tskit. Most importantly, the JBLB magic bytes should either (a) be exposed in core.h, or (b) should be set up by a tskit function, probably? It's a bit weird for SLiM to duplicate this magic in its own code. Less importantly, it'd be nice to provide set_u64_le() as a utility function. Providing a "write" equivalent of tsk_json_struct_metadata_get_blob() seems tricky since it wouldn't want to do its own malloc for the result, so then the caller needs to know how many bytes of storage will be needed, so life gets complicated and messy. Anyhow, I'll just duplicate the magic and set_u64_le() in SLiM for now, but maybe this is worth thinking about a bit so that every tskit client using this stuff doesn't end up copying those same things into their own codebases?

[jeromekelleher]

I don't know if this is really necessary - we've defined the protocol here, and it's not going to change. I think it would be simpler for you to implement functions to your own taste in SLiM @bhaller and to cut out the middleman. These are all pretty standard things that can be implemented straightforwardly in the style suitable to a local implementation.

Please reopen if you have a specific function you'd like to see implemented.

[gregorgorjanc]

FWIW We might hit these issues once we are in the guts of working with AlphaSimR & tree sequence recording with metadata, but I don't understand the details that @bhaller is raising here. Maybe we cross that bridge when we get there and we provide PRs with desired behaviour;)

[bhaller]

I don't know if this is really necessary - we've defined the protocol here, and it's not going to change.

Well, exactly – that's my point. The JBLB magic bytes are already defined in core.c as:

static const uint8_t TSK_JSON_BINARY_MAGIC[4] = { 'J', 'B', 'L', 'B' };

That is part of the defined protocol for this codec, and is needed by clients in order to write data for this codec correctly. All that is needed is to remove the static here and add an extern declaration for this in core.h. Without that, clients have to duplicate these magic bytes in their code, which doesn't make sense since it is part of the definition of this codec, and should therefore be public API. I don't understand the pushback here.

I think it would be simpler for you to implement functions to your own taste in SLiM @bhaller and to cut out the middleman. These are all pretty standard things that can be implemented straightforwardly in the style suitable to a local implementation.

Please reopen if you have a specific function you'd like to see implemented.

As I wrote above, there is a specific function I'd like to see implemented: set_u64_le(). Of course this is not rocket science, and in any case I have already copied it into SLiM. The point is, again, that any client writing data for this codec will need this function, and it is already in tskit's code base (presently in the test code), so why not just make things easier for everyone, and reduce code duplication, by making it public?

The point is that tskit provides tsk_json_struct_metadata_get_blob(), but does not (and should not) provide a tsk_json_struct_metadata_set_blob() function – and so it should provide the standard moving parts needed for a client to do that work themselves, for convenience. That is TSK_JSON_BINARY_MAGIC and set_u64_le(). (By the way, the name tsk_json_struct_metadata_get_blob() is probably not the right function name any more, since it now gets both the JSON and the binary blob – it should probably have been renamed when that functionality was added by @benjeffery .)

Neither of these things is a big deal. SLiM already has them duplicated in its code. I'm just making suggestions – as requested – that would improve the API associated with this codec and make it easier for future clients, like pyslim and AlphaSimR, to use it. If you want to close this, then close it, but I don't understand why you would do that.

[jeromekelleher]

So why not just duplicate the bytes? It's one line of code, and, as I said, it's not going to change.

Generic functions like set_u64_le are out of scope for tskit - it's not our job to maintain general functions like this.

Unless here's a genuine issue here that needs to be resolved I think can we please close this one and get on with doing other things.

[bhaller]

A flawed API design is a genuine issue. But I give up.