Codegen builders / setters / child validators

[DeanPDX]

Hey @toptobes I took a stab at integrating your idea into the repo. I decided to just make Validate() optional and check for it. I avoided using reflection and wrote a benchmark to verify that whatever we do, performance doesn't suffer.

Still need to figure out what we are going to do about comments. Working on that.

[toptobes]

Nice! Will take a look in a bit

Fwiw though I wouldn't worry too much about benchmarks here

• The vast majority of the time the options objects will be small and only shallowly nested at most
• The validation code isn't called super often (in the scale of computer time) anyways
• The execution time of the options logic is dwarfed by the API call anyhow
• Benchmarks aren't super reliable, especially when not run in the proper environment under proper conditions

[DeanPDX]

Yeah - you could argue that benchmarks are premature optimization in this case. But - they are really easy to write in go and benchmarking is pretty prevalent in the community as a result.

[DeanPDX]

Btw this closes #17. Also closes #15.

[toptobes]

I see nothing egregious, and it's really satisfying seeing all of the now-old boilerplate being removed 🙂

[toptobes]

Suggested change

	merged, _ := MergeOptions(v...)
	o.{{ .Field }} = merged
	o.{{ .Field }} = MergeOptions(v...)

I thinking pulling out MergeOptions into

• MergeOptions<T> => T (sets defaults + merges options; used inside builders)
• MergeAndValidateOptions<T> => T, error (calls the prev function + validates; used inside commands)

could be a bit clearer + save on a few CPU cycles since the error isn't being used here anyways

[DeanPDX]

Not a bad thought! Also - now that I'm thinking about this, we could probably shorten that to Merge since, outside the package, we are using it like this:

// Current Options is redundant
merged, err := options.MergeOptions(opts...)
// Updated
merged, err := options.Merge(opts...)

Which make options.MergeAndValidate not seem as verbose.

[toptobes]

doesn't matter that much maybe "Setters" or "ListSetters" may be the most clear name

[DeanPDX]

Yeah - naming things is hard. I think "Builder" makes sense for the overarching idea that it "builds" options. But the funcs themselves being called "Setters" might make more sense. More than anything, I want to think about how it looks/feels to consumers of the library. Consider this godoc link for create keyspace:

https://pkg.go.dev/github.com/datastax/astra-db-go#AstraDbAdmin.CreateKeyspace

I don't love that the method signature is:

CreateKeyspace(ctx context.Context, keyspace string, opts ...options.Builder[options.CreateKeyspaceOptions]) error

It's not obvious to the caller that they can use the fluent builder / pass options directly without more comments. And I HAVE covered that in comments / examples but I am iffy about this in the godoc. So one option would be aliasing them:

// CreateKeyspaceOption configures a CreateKeyspace operation.
// Use the fluent builder:
//
//	options.CreateKeyspace().SetBlocking(false)
//
// Or pass a struct directly:
//
//	&options.CreateKeyspaceOptions{Blocking: ptr.To(false)}
type CreateKeyspaceOption = Builder[CreateKeyspaceOptions]

Then we change our CreateKeyspace method to:

CreateKeyspace(ctx context.Context, keyspace string, opts ...options.CreateKeyspaceOption) error

Or something along those lines. But basically, I want to get builder out of that method signature altogether if possible.

[toptobes]

major nit but leaving a gap between the boilerplate comment and the actual comment may be more readable in IDEs, especially when the lines are not split

Suggested change

	// {{ .Method }} sets the {{ .Field }} option.
	// {{ .Comment }}
	// {{ .Method }} sets the {{ .Field }} option.
	//
	// {{ .Comment }}

[DeanPDX]

I went back and forth on this one. I tried it both ways, and I think it often reads BETTER with things crunched together a bit. Current:

VS added space:

And here on a longer / more detailed comment:

Basically, it naturally reads "sets property x. Property x does y".

[toptobes]

Hm you know what fair enough

[DeanPDX]

Hey @toptobes I made some more changes. Renamed builder, added those aliases to codegen, and then I realized we can just NOT export the builders at all for decreased surface area. It cleans up the godoc to not have those comments codegen copy-pasted 2 places.

Only thing I need to do still is: fix the example for the aliases. Right now it's:

// CreateKeyspaceOption is a convenience alias for Builder[CreateKeyspaceOptions].
type CreateKeyspaceOption = Builder[CreateKeyspaceOptions]

... which I did just to vet the idea and get the whole thing building. But need to change it to be more descriptive like we talked about:

// CreateKeyspaceOption configures a CreateKeyspace operation.
// Use the fluent builder:
//
//	options.CreateKeyspace().SetBlocking(false)
//
// Or pass a struct directly:
//
//	&options.CreateKeyspaceOptions{Blocking: ptr.To(false)}
type CreateKeyspaceOption = Builder[CreateKeyspaceOptions]

Once I figure that out, we will be in real good shape.

[DeanPDX]

Hey @toptobes I figured out the last piece. I started moving towards stringers instead of everything being in one giant template. I think I might refactor some of the other things to be a little more self-contained as well. Also makes them easier to test (I added some tests as well).

Here's what the godoc looks like:

Hyperlinks referencing related types everywhere. Code examples you can use. And the struct defs are kept close to each other. Etc. I think it's pretty good. Ready to move on to getting more coverage and keep using this for now and see when I run up against any rough edges in the future and fix them.