Move endpoint and endpoint test generation to module system #81
Conversation
module/module.go
Outdated
| ) | ||
| } | ||
|
|
||
| n, err := file.Write(bytes) | ||
|
|
||
| if err != nil { | ||
| return errors.Wrapf(err, "Error writing to file %s", filePath) | ||
| return errors.Wrapf(err, "Error writing to file \"%s\"", filePath) |
There was a problem hiding this comment.
There is a %q that will encode with the quotes. Aka %q is "%s"`
There was a problem hiding this comment.
nice, good to know, replaced them
ChuntaoLu
force-pushed
the
lu.module
branch
2 times, most recently
from
047a0cb
to
48bd0ec
Compare
There was a problem hiding this comment.
The configuration for all module types is supposed to live in the "config:" key. The "endpoints" array needs to live on that config object. The endpoint generation modules should generate both endpoints and tests from that config.
Also I don't think it's that clean to move all of the endpoint config and template structs into the module_system.go file. That file is a clean expression of a) the module types, i.e. the first section describes the structure of the repository, and b) the generator implementation that calls into templates. I think the actual config structs should remain in the gateway.go file for now, and we should move them into separate files per module type later. I think the volume of code per module warrants that at this point.
I should have noted the config arrangement in this PR is tentative, the main focu was to add endpoint generation into the module system with minimum config structure change. As to the actual config structure, we should come up with a proper schema before diving into change those json files.
Those config structs were originally in template.go, which is going away as the module system takes care of code generation. I agree with the point of what module_system.go files tries to tell, I don't think it matters too much where those structs are for now since things will surely gets moved around a lot. |
| func NewDefaultModuleSystem( | ||
| configDirName string, | ||
| middlewareConfig string, | ||
| h *PackageHelper, |
There was a problem hiding this comment.
Have you considered putting the middleware config information on the PackageHelper. To me it feels a bit weird to have these arguments here. Right now I have a codebase that I only generate clients in, and the change to this interface would now require me to care about non-existent middleware configs.
There was a problem hiding this comment.
Honestly, I didn't know a good way to get a hold of the middleware config, I know it feels awkward to pass in as arguments, it doesn't feel belong to PackageHelper either.
There was a problem hiding this comment.
I don't feel like it belongs in either place, but the PackageHelper gives us information about the structure of the package. With middlewares in this current mode, they are a permanent fixture of the package, which is arguably why the package helper does make some sense. It also has the benefit that existing consumers initialize the package helper in the current way and this configuration can be defaulted.
codegen/module_system.go
Outdated
| @@ -92,10 +100,68 @@ func NewDefaultModuleSystem(h *PackageHelper) (*module.System, error) { | |||
| ) | |||
| } | |||
|
|
|||
| // TODO: Register endpoint module class and type generators | |||
| middlewareSpecs := map[string]*MiddlewareSpec{} | |||
There was a problem hiding this comment.
I think the middlewares need to be modeled as a dependency but we can think about that later. It might make sense to treat them as a global grab bag (i.e. a single module of middlewares)
There was a problem hiding this comment.
Agreed, I like them to be explicitly specified as an endpoint's dependencies, so module system can pick them up just like clients. But they are slightly different because middleware config may need to provide input for middlewares to work.
|
👍 lgtm |
No description provided.