Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

v3: Remove all replace-type functionality #864

Closed
LandonTClipp opened this issue Dec 26, 2024 · 5 comments
Closed

v3: Remove all replace-type functionality #864

LandonTClipp opened this issue Dec 26, 2024 · 5 comments
Labels

Comments

@LandonTClipp
Copy link
Collaborator

This was needed to get around type aliases, but the Go AST fully supports aliases now so there is no need to have this anymore.

@RangelReale
Copy link
Contributor

I found an edge case that may require that this parameter be kept:

x/internal/options.go

package internal

type Options struct {
	Dir string
}

x/itf.go

package x

import "sample/x/internal"

type IType interface {
	Calculate(options internal.Options)
}

Generating a mock outside the x package (or if this was an external package), the mock for the Calculate function would generate an invalid import to the internal package.

It may be an edge case, but I found this problem on the go.temporal.io/sdk/client package: the WorkflowRun type is an alias to an internal type, and its GetWithOptions method has a parameter that is an internal type.

The internal option type is also exported in the same root file, but mockery has no way to know this, so I'm using replace-type to fix this.

Granted, this is not a very good design, but it is legal Go code, and may happen in other codebases.

@LandonTClipp
Copy link
Collaborator Author

Hmm that's surprising to me that it's valid Go. We should probably create a fixture for it and add it to the tests. I'll look into adding it back.

@LandonTClipp
Copy link
Collaborator Author

I can't think of a way around this, it's frustrating that someone would design an API like that! Alas, I will have to add it back.

LandonTClipp added a commit to LandonTClipp/mockery that referenced this issue Jan 20, 2025
@LandonTClipp
Copy link
Collaborator Author

I have a working PR. Porting to v3 wasn't straightforward due to the requirements that the templates receive real type information, so we basically have to dynamically parse the referenced type. Had to change the schema of the parameter to make this easier/cleaner.

Just need to add some tests and we'll be good. Only thing yet to be done is the support for replacing generic type parameters.

LandonTClipp added a commit that referenced this issue Jan 20, 2025
* Add back `replace-types`

Port the `replace-type` parameter into mockery v3. This changes the config schema used in the v2 replace-type parameter to be more grokable. This is also a practical matter: the somewhat complex parsing mechanism in v2 is replaced by simple struct attributes.

The implementation here is quite different from v2. The templating system in v3 requires type information for all types, and thus we cannot do simple string replacements. We have to call `packages.Load` on the referenced type so we can get real type information. This means that `replace-type` will incur additional latency. This latency penalty, however, grants us additional correctness of implementation: the mock templates will have full type information, and if something about the `replace-type` parameter is wrong (either package path or type name don't exist), mockery will explicitly log this as an error.

This does _not_ implement replacements of type constraints as done in v2: #750. This work still needs to be done.

Per #864

* Additional documentation

* Add docs and hint in log message on how to enable force-file-write.
@LandonTClipp
Copy link
Collaborator Author

Closing this as "won't do" since I re-implemented replace-type.

@LandonTClipp LandonTClipp closed this as not planned Won't fix, can't repro, duplicate, stale Jan 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants