-
Notifications
You must be signed in to change notification settings - Fork 405
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
Add ability to replace type constraints #750
Conversation
Excellent! I'll give it a proper review in a few days. Thanks for the contribution. |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #750 +/- ##
===================================================
+ Coverage 42.45067% 42.65889% +0.20821%
===================================================
Files 61 63 +2
Lines 4815 4972 +157
===================================================
+ Hits 2044 2121 +77
- Misses 2591 2653 +62
- Partials 180 198 +18 ☔ View full report in Codecov by Sentry. |
Hey @LandonTClipp Replace a type parameter: type Foo[T any] struct {}
func (*Foo) Get() T {} type Foo[T baz.Baz] struct {}
func (*Foo) Get() T {} Remove a type parameter and set each instance of it: type Foo[T any] struct {}
func (*Foo) Get() T {} type Foo struct {}
func (*Foo) Get() baz.Baz {} |
93da8ee
to
4b08243
Compare
I've updated the API to the suggestion above which I'm using successfully for my usecase and hopefully it provides more flexibility for others. I'm happy to change the API if you have any ideas. |
Hi @DustinJSilk, apologies for the lack of responses the last few weeks. I have been on the hunt for a new job and I haven't been able to devote much time or energy to this project, but luckily my search is coming to a close (hopefully, knock on wood). As far as your proposals, I think I'm on board with it. The only thing I'm unhappy about (which has nothing to do with your proposal or implementation) is that I don't enjoy these complex, custom parameter strings. I think in hindsight, I would have preferred |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, I think I'm okay with the new format here. I have a few comments, if we can get those addressed then I will merge it!
pkg/generator.go
Outdated
// Match type parameter substitution | ||
match := regexp.MustCompile(`\[(.*?)\]$`).FindStringSubmatch(t) | ||
if len(match) >= 2 { | ||
if match[1][:1] == "-" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was initially confused why you did that instead of:
match[1][0]
but realized the slicing operator will return a string instead of a single byte. To make this cleaner, I'd instead do this:
if strings.HasPrefix(match[1], "-")
It's a bit more obvious what's happening.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me know if it still makes sense for you, i was able to put it in one line with strings.CutPrefix
pkg/generator_test.go
Outdated
func (s *GeneratorSuite) TestReplaceTypeGeneric() { | ||
cfg := GeneratorConfig{InPackage: false, ReplaceType: []string{ | ||
"github.com/vektra/mockery/v2/pkg/fixtures.ReplaceGeneric[-TImport]=github.com/vektra/mockery/v2/pkg/fixtures/redefined_type_b.B", | ||
"github.com/vektra/mockery/v2/pkg/fixtures.ReplaceGeneric[TConstraint]=github.com/vektra/mockery/v2/pkg/fixtures/constraints.Integer", | ||
"github.com/vektra/mockery/v2/pkg/fixtures.ReplaceGenericSelf[-T]=github.com/vektra/mockery/v2/pkg/fixtures.*ReplaceGenericSelf", | ||
}} | ||
|
||
s.checkGenerationRegexWithConfig("generic.go", "ReplaceGeneric", cfg, []regexpExpected{ | ||
// type ReplaceGeneric[TConstraint constraints.Integer, TKeep interface{}] struct | ||
{true, regexp.MustCompile(`type ReplaceGeneric\[TConstraint constraints.Integer\, TKeep interface\{\}] struct`)}, | ||
// func (_m *ReplaceGeneric[TConstraint, TKeep]) A(t1 test.B) TKeep | ||
{true, regexp.MustCompile(`func \(_m \*ReplaceGeneric\[TConstraint, TKeep\]\) A\(t1 test\.B\) TKeep`)}, | ||
// func (_m *ReplaceGeneric[TConstraint, TKeep]) B() test.B | ||
{true, regexp.MustCompile(`func \(_m \*ReplaceGeneric\[TConstraint, TKeep\]\) B\(\) test\.B`)}, | ||
// func (_m *ReplaceGeneric[TConstraint, TKeep]) C() TConstraint | ||
{true, regexp.MustCompile(`func \(_m \*ReplaceGeneric\[TConstraint, TKeep\]\) C\(\) TConstraint`)}, | ||
}) | ||
|
||
s.checkGenerationRegexWithConfig("generic.go", "ReplaceGenericSelf", cfg, []regexpExpected{ | ||
// type ReplaceGenericSelf struct | ||
{true, regexp.MustCompile(`type ReplaceGenericSelf struct`)}, | ||
// func (_m *ReplaceGenericSelf) A() *ReplaceGenericSelf | ||
{true, regexp.MustCompile(`func \(_m \*ReplaceGenericSelf\) A\(\) \*ReplaceGenericSelf`)}, | ||
}) | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not like these kinds of tests and am asking people not to do them. Any changes to the formatting can/will cause these kinds of tests to fail. Instead I prefer if we just create a new test file, probably in pkg/fixtures/test
that checks these various scenarios through a functional test, rather than string comparison. You can just rely on the compiler to fail if the param types are mismatched, or maybe use type assertion through an empty interface in the test (actually, type assertion might be better in this case so that we don't cause all the tests to fail if the mock implementation is wrong somehow).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea! I've added some better test cases and removed these. I didn't quite figure out exactly how we could test just with type assertions, at the moment the compiler would complain if the types no longer match
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent, this looks good to me. Thanks again!
The tests are failing due to an unrelated bug that I will fix in a separate PR. |
Good work @DustinJSilk, feature has been released. |
Amazing thanks @LandonTClipp |
Description
Adds a config option that can replace generic types with a concrete type.
Type of change
Version of Golang used when building/testing:
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Checklist