schema: make optional and nullable SpawnFoo parameters a bit safer

[mvdan]

Right now we have:

func SpawnStructField(name string, typ TypeName, optional bool, nullable bool) StructField

Nothing wrong with it per se, and it works. The code then looks like:

ts.Accumulate(schema.SpawnStruct("StructRepresentation_Map_FieldDetails",
    []schema.StructField{
        schema.SpawnStructField("rename", "String", true, false),
        schema.SpawnStructField("implicit", "AnyScalar", true, false),
        },
    schema.StructRepresentation_Map{},
))

Can a reader quickly tell what each true and false means? I definitely can't. Every time, I have to go check the SpawnStructField definition to remind myself if optional or nullable are first.

Other APIs have similar issues; SpawnList takes a boolean, and unless you're very used to this API, it's not evident that it means "nullable".

Another kind of unfortunate mistake is that one can mistakenly pass variables in the wrong places, and it won't be evident or caught by the typechecker:

schema.SpawnStructField("oops", "Oops", myNullable, myOptional),

I propose to make code using Spawn funcs easier to use by making them take named boolean parameters, like:

type OptionalBool bool

const (
    NonOptional OptionalBool = false
    Optional OptionalBool = true
)

type NullableBool bool

const (
    NonNullable NullableBool = false
    Nullable NullableBool = true
)

func SpawnStructField(name string, typ TypeName, optional OptionalBool, nullable NullableBool) StructField

Now, the problematic lines from above can be much clearer:

// fully explicit.
schema.SpawnStructField("rename", "String", schema.Optional, schema.NonNullable),

// only explicit when non-default.
schema.SpawnStructField("rename", "String", schema.Optional, false),

// older and less safe versions will still be allowed, thanks to consts not needing explicit type conversions.
// also means we almost certainly don't break any downstreams.
schema.SpawnStructField("rename", "String", true, false),

// using vars the wrong way would be obvious, given non-consts require explicit type conversions.
schema.SpawnStructField("oops", "Oops", schema.Optional(myNullable), schema.Nullable(myOptional)), // wrong!

[mvdan]

@warpfork and I spoke about this a few weeks ago - it might not even be worthwhile trying to make the Accumulate/Spawn APIs nicer to use if practically all users will move to the schema parser and compiler.

It's also worth noting that the current Spawn APIs lack features or are outright outdated. For example, support for any is missing, as well as support for inline type definitions for e.g. struct fields.

So I see two paths forward:

1. We move the Accumulate/Spawn APIs to be internal-only, to be used by the ipld-prime packages (schema-schema bootstrap, dmt.Compile). End users need to use the high-level Parse and Compile APIs to declare schemas, and that's our stable API.

2. We make the schema API implement the full schema spec, while fixing some of the sharp edges as well. When we do that, then I think changes like what I suggest here would make sense.

I'd prefer if we went with option 1 instead of 2, in which case sharp edges in the API aren't really a worry, because they will be internal APIs. I imagine that most users of Accumulate+Spawn could transition to ipld.LoadSchema over the next weeks/months.

[masih]

Sounds good , probably should move this ticket to backlog?

[mvdan]

Indeed. My bad for not adding the extra context. When I filed this issue, I thought the schema package was complete, and I just wanted to polish its rough edges a bit.

[mvdan]

Closing for the same reasons as #208 (comment): long term, we expect everyone to load a schema either in DSL or DMT form. The only place that would need to manually "spawn" a schema is the bootstrapping of the schema-schema itself, in the DMT package.

Once the "dsl parse" and "dmt compile" packages support all schema features, then I think we could move the "spawn" APIs so they are hidden from external users, but still available to the dmt package.

[mvdan]

I should add: even for an internal "schema spawner" API, we'll likely want to tweak and improve the API somewhat. But it's not a priority, and we won't need to worry about end-user UX for a private API, which is what this issue was about.