Closed applejag closed 6 months ago
This looks really promising!
This solution aims to be opt-in on a per-function basis, so it should not be as intrusive.
I think this is a key point. This should be great as something that's available but not required.
I'll review in more detail. Awesome stuff.
I haven't checked the code yet and I like the idea of having a tool to generate the boilerplate, like cobra
and kubebuilder
do but I'm not so sure about the drawbacks mentioned in the PR description, they seem pretty significant to me and sound like something that would get in the way of my workflow.
I haven't checked the code yet and I like the idea of having a tool to generate the boilerplate, like
cobra
andkubebuilder
do but I'm not so sure about the drawbacks mentioned in the PR description, they seem pretty significant to me and sound like something that would get in the way of my workflow.
I listed the drawback together with counterpoints to the drawbacks in the same bulletpoints.
Go's build system isn't super flexible, so forcing you to run go generate
is pretty much the best we can do. It's also the approach taken by lots of other projects, such as gRPC, templ, and kubebuilder as you mentioned.
Regarding the layer of abstraction one, the counterpoint to that one is that if you want full control, then you just implement the function yourself without the generated mapping. The way it's done in this PR means that you can mix and match generated and non-generated functions freely in the same module.
I think a bit of duplication and some copy/pasting is better than adding complexity and forcing a specific workflow.
As for kubebuilder
I was talking specifically about the boilerplate generator part, not the manifest and deep copy generation part and you are not required to run dose on every edit for sure.
I guess this case should be closer to what cobra does for adding new commands, it generates a starting point for you to edit and that's it, I think it would be quite simple to do that for a new module and no changes would be needed on the existing code for that.
It seems like the approach prototyped here doesn't force a specific workflow, in that the module author can choose to opt-in using the risor:generate
comment.
Do you agree @luisdavim?
It seems like the approach prototyped here doesn't force a specific workflow, in that the module author can choose to opt-in using the
risor:generate
comment.Do you agree @luisdavim?
Yes, looks like it. I still think that something that would generate a starting point to be edited would be simpler and more useful.
This approach looks very cool but I think it will end up serving more limited use cases. But we can add that later and I don't see any issues in adding this as it can be useful for those simpleer use cases.
I think the generated code should be committed, and there should be a make target for triggering the code generation.
The generated code is committed, and I've now added two targets:
make generate
to just run go generate
make modgen
to run ./cmd/risor-modgen
via entr
Writing Risor Go modules (aka builtin modules) is a lot of boilerplate.
This is a proposal to automate some parts of that, by writing our own Go code generator.
To start with, I've only implemented some core features, just so I can try it out. I've also replaced most functions in the
strings
module with these generated ones. To replace the remaining functions in thestrings
module too, I'd just have to add support for slice types, which isn't a biggy.Some features that I did not implement just to keep the initial PR smaller:
error
returns and(any, error)
into correctobject.NewError
return.func(...any)
[]string
,[]byte
,map[string]any
,SomeCustomType
exec.command
Benefits
Drawbacks
go generate
command after every edit. Could be paired withentr
like inmake docs-dev
, and should be added to CI pipelines to ensure it's not forgotten.func(context.Context, ...object.Object) object.Object
to whatever the function declares. Can be fixed by for example having the function with//risor:export
being the private function.Closes #158