Preserve extension fields when marshalling/unmarshalling

[cpuguy83]

This makes it so external callers don't need to handle this themselves.

[cpuguy83]

Converting this to draft since I realized it really doesn't help much except for when the extension field value is a core type.

[pmengelbert]

See the above comment. At the very least, we need to give it some serious thought before adding private fields with abstract data types into the main Spec struct.

[pmengelbert]

nit

Suggested change

	parsed, err := parser.ParseBytes(dt, 0)
	const noFlags = 0
	parsed, err := parser.ParseBytes(dt, noFlags)

[pmengelbert]

ditto

[pmengelbert]

I don't like adding private fields to the spec struct, especially abstract stuff like *ast.File. We worked hard early on to ensure that the spec remained simple data, rather than data plus functionality.

For example, we decided not to implement the Source type as a group of interfaces because we wanted to maintain the spec as simple data. This also means that anyone importing the spec object will be importing goccy/go-yaml/ast.

I recognize that it will be a pain to wrap the spec and change function signatures, but a lot of that could be done with a find/replace operation and having the first member of the wrapper type be the spec:

type specExtensions struct {
        extRaw *ast.File
        ext map[string]interface{}
}

type SpecWithExtensions struct {
        *Spec
        ext specExtensions
}

[cpuguy83]

Spec is just a data type.
Currently it is unable to represent all the data that can go into a spec since we can't dynamically generate fields for extension fields.

These changes make it so Spec can represent that data and make it accessible to those who want it.
Its not changing the actual dalec spec in any, is not exposed outside of this package.

What concerns do you have for this?

[pmengelbert]

I think the spec should remain simple data with no dependencies. I think if we want to provide the other information, we should expose some other mechanism to the user.

I don't like taking on a dependency on an external library for the spec data type. For something as important and user-facing as the spec data structure, I want the Dalec maintainers to maintain complete control over it. I want a clear boundary between our structural definition and external code.

I don't like mixing "simple data" (kv pairs with nesting) with more abtract representations of concepts like *ast.File. I don't like it because it makes things more complex. By "more complex", I mean harder to follow and harder to reason about.

This might not be a big deal in isolation. But I think these not-so-big deals add up in the long run, and compromise the maintainability of the project.

What I propose is a second public function, func ParseSpecWithExtensions(b []byte) (SpecWithExtensions, error) that returns a struct that wraps the spec and adds the additional data types.

[cpuguy83]

There mere act of even ignoring these fields during unmarshal requires access to the AST or other trickery.
Having the *ast.File in the spec type is purely an optimization (note, it is not part of the spec, just the data type we are using to represent the data the spec already allows for). It could be a []byte, but then we'd need to parse it again anytime someone wants to extract one of these extension fields, and we'd be parsing it to an *ast.File.

We are also already dependent on an external library to even encode and decode the plain spec (even without extensions).
It's there, we don't expose it anywhere (nor does this change expose it), but its there.

This change allows raw calls to yaml.Unmarshal to just work, and should work even if the caller is using a different yaml library.

[pmengelbert]

LGTM