Add support for overriding configuration through comments on interfaces #1105
Conversation
config/interface_overrides.go
Outdated
| overrides.FileName = strings.TrimSpace(matches[1]) | ||
| } else if matches := templateRe.FindStringSubmatch(text); len(matches) > 1 { | ||
| overrides.Template = strings.TrimSpace(matches[1]) | ||
| } |
There was a problem hiding this comment.
This is really good, but I think we can do even better. Obviously the problem with this way of doing it is that we must have manual support for each individual config parameter. Instead, I think we could design this in a way that it will support all config parameters as defined in config.Config.
The way you'd want to go about this is to parse the yaml in the doc comment into this config struct, then merge this into the interface's Config struct. You already kind of do this below in your Override method, but it's cleaner to use the merge method already available.
Additionally, it'd be better if the override discovery logic lives here in GetInterfaceConfig instead of needing a separate call. I'd eventually like to bring the showconfig command into parity with what you've written here so that overrides are properly represented. Obviously you'd need to pass the *ast.GenDecl syntax to make that work, which is fine.
Overall great work, thank you for tackling this!
There was a problem hiding this comment.
Sorry It took me so long for getting back to this! Yeah, I totally agree that this was a little bit too manual. I've refactor it a little bit to extract a yaml from the comment section and parse it into a config - merging it with the file based pkg/interface config if they exist.
Regarding the last piece:
Additionally, it'd be better if the override discovery logic lives here in GetInterfaceConfig instead of needing a separate call.
If we were to do that, we wouldn't have loaded the config from the comments before the ShouldGenerateInterface check, this means that we would need to have the interface defined in the config file (unless we move this check to happen later).
config/interface_overrides.go
Outdated
| generateRe = regexp.MustCompile(`mockery_generate: (true|false)`) | ||
| structNameRe = regexp.MustCompile(`mockery_struct_name: ([^\s]+)$`) | ||
| fileNameRe = regexp.MustCompile(`mockery_file_name: ([^\s]+)$`) | ||
| templateRe = regexp.MustCompile(`mockery_template: ([^\s]+)$`) |
There was a problem hiding this comment.
The go doc language spec talks about directives and states that
Tools that define their own directives should use the form //toolname:directive.
It seems like the proposed directives should conform to this syntax too, so comments would be:
//mockery:<parameter name> <value>If that makes sense?
There was a problem hiding this comment.
That makes a lot of sense. I'm not sure if this PR is going to be updated, we might need someone else to take it over.
There was a problem hiding this comment.
Hey 👋 (sorry for being missing)!
I like that! I took a chance to refactor the initial pr with @LandonTClipp initial comment and this suggestion! Let me know what do you think.
|
|
||
| // Requester is an interface that defines a method for making HTTP requests. | ||
| // | ||
| //mockery:generate: true |
There was a problem hiding this comment.
My reading of the code is that the generate: true config is not actually doing anything other than serving as a trigger that says "hey, there is a mockery: directive here". We could have put anything here, like //mockery:foo: bar and the same thing would happen. Is that accurate? If so, the generate: config should have a real meaning, as in, the presence of this config (and not others) triggers the mock to be generated. Furthermore, I think this should really just be //mockery:generate to conform with the same standard as //go:generate.
3 participants
Description
Adds support for configuring mocks through directive comments on the interface. The comment will be parsed as a yaml configuration, any configuration supported on the file based config is also supported by this:
These configuration will take precedene of over any other configuration.
I haven't made any changes to documentation yet as I wanted to wait for a confirmation that this pr will be accepted before doing so.
Type of change
Version of Go used when building/testing:
How Has This Been Tested?
Added a new package with these annotations in the fixture folder, testing all the new configurations.
Checklist