-
Notifications
You must be signed in to change notification settings - Fork 35
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
Proposal for generics-based Setter #201
base: master
Are you sure you want to change the base?
Changes from all commits
9f4a117
55cb412
c99bf26
1dc5951
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,6 +20,7 @@ import ( | |
|
||
"github.com/mailgun/holster/v4/setter" | ||
"github.com/stretchr/testify/assert" | ||
"github.com/stretchr/testify/require" | ||
) | ||
|
||
func TestIfEmpty(t *testing.T) { | ||
|
@@ -145,3 +146,151 @@ func TestIsNil(t *testing.T) { | |
assert.True(t, setter.IsNil(thing)) | ||
assert.False(t, setter.IsNil(&MyImplementation{})) | ||
} | ||
|
||
// --------------------------------------------------------- | ||
|
||
var newStrRes string | ||
|
||
func BenchmarkSetterNew(b *testing.B) { | ||
var r string | ||
for i := 0; i < b.N; i++ { | ||
setter.Default(&r, "", "", "42") | ||
} | ||
newStrRes = r | ||
} | ||
|
||
var oldStrRes string | ||
|
||
func BenchmarkSetter(b *testing.B) { | ||
var r string | ||
for i := 0; i < b.N; i++ { | ||
setter.SetDefault(&r, "", "", "42") | ||
} | ||
oldStrRes = r | ||
} | ||
|
||
var newSliceRs []string | ||
|
||
func BenchmarkSetterNew_Slice(b *testing.B) { | ||
r := make([]string, 0, 3) | ||
b.ResetTimer() | ||
for i := 0; i < b.N; i++ { | ||
setter.DefaultSlice(&r, []string{}, []string{"welcome all", "to a benchmark", "of SILLY proportions"}) | ||
} | ||
newSliceRs = r | ||
} | ||
|
||
var oldSliceRs []string | ||
|
||
func BenchmarkSetter_Slice(b *testing.B) { | ||
r := make([]string, 0, 3) | ||
b.ResetTimer() | ||
for i := 0; i < b.N; i++ { | ||
setter.SetDefault(&r, []string{""}, []string{"welcome all", "to a benchmark", "of SILLY proportions"}) | ||
} | ||
oldSliceRs = r | ||
} | ||
|
||
func TestSetterNew_IfEmpty(t *testing.T) { | ||
var conf struct { | ||
Foo string | ||
Bar int | ||
} | ||
assert.Equal(t, "", conf.Foo) | ||
assert.Equal(t, 0, conf.Bar) | ||
|
||
// Should apply the default values | ||
setter.Default(&conf.Foo, "default") | ||
setter.Default(&conf.Bar, 200) | ||
|
||
assert.Equal(t, "default", conf.Foo) | ||
assert.Equal(t, 200, conf.Bar) | ||
|
||
conf.Foo = "thrawn" | ||
conf.Bar = 500 | ||
|
||
// Should NOT apply the default values | ||
setter.Default(&conf.Foo, "default") | ||
setter.Default(&conf.Bar, 200) | ||
|
||
assert.Equal(t, "thrawn", conf.Foo) | ||
assert.Equal(t, 500, conf.Bar) | ||
} | ||
|
||
func TestSetterNew_Slices(t *testing.T) { | ||
var foo []string | ||
require.Len(t, foo, 0) | ||
|
||
// Should apply the default values | ||
setter.DefaultSlice(&foo, []string{"default"}) | ||
require.Len(t, foo, 1) | ||
assert.Equal(t, "default", foo[0]) | ||
|
||
foo = []string{"thrawn"} | ||
|
||
// Should NOT apply the default values | ||
setter.DefaultSlice(&foo, []string{"default"}) | ||
require.Len(t, foo, 1) | ||
assert.Equal(t, "thrawn", foo[0]) | ||
} | ||
|
||
func TestSetterNew_IfDefaultPrecedence(t *testing.T) { | ||
var conf struct { | ||
Foo string | ||
Bar string | ||
} | ||
assert.Equal(t, "", conf.Foo) | ||
assert.Equal(t, "", conf.Bar) | ||
|
||
// Should use the final default value | ||
envValue := "" | ||
setter.Default(&conf.Foo, envValue, "default") | ||
assert.Equal(t, "default", conf.Foo) | ||
|
||
// Should use envValue | ||
envValue = "bar" | ||
setter.Default(&conf.Bar, envValue, "default") | ||
assert.Equal(t, "bar", conf.Bar) | ||
} | ||
|
||
func TestSetterNew_IsEmpty(t *testing.T) { | ||
var count64 int64 | ||
var thing string | ||
|
||
// Should return true | ||
assert.Equal(t, true, setter.IsZeroNew(count64)) | ||
assert.Equal(t, true, setter.IsZeroNew(thing)) | ||
|
||
thing = "thrawn" | ||
count64 = int64(1) | ||
assert.Equal(t, false, setter.IsZeroNew(count64)) | ||
assert.Equal(t, false, setter.IsZeroNew(thing)) | ||
} | ||
|
||
// Not possible now given compiler warnings | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In what Go version? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a good point - this change would not work pre-Go generics, as far as I am aware. So Go 1.18+? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We are not running it in Go 1.18 - https://github.com/mailgun/holster/blob/master/.github/workflows/test.yaml#L19-L20 Are you seeing these warnings on your local env? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's correct. And that's why these tests are commented out. It's not possible to do what these tests did previously with the new methods. That's what its demonstrating 👀 Mind you, the name of the methods changed. I can fix that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And to be explicit: this test was previously trying to assign an int to a string, which you technically could do in the original method. The generics form doesn't allow that. It's a compilation error There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I got it now. Yeah, we don't need them at all, please remove the commented code. https://kentcdodds.com/blog/please-dont-commit-commented-out-code There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep, all that will be removed once we settle on the PR final form 👍 I just left them there as a note to reviewers! |
||
// func TestSetterNew_IfEmptyTypePanic(t *testing.T) { | ||
// defer func() { | ||
// if r := recover(); r != nil { | ||
// assert.Equal(t, "reflect.Set: value of type int is not assignable to type string", r) | ||
// } | ||
// }() | ||
|
||
// var thing string | ||
// // Should panic | ||
// setter.Default(&thing, 1) | ||
// assert.Fail(t, "Should have caught panic") | ||
// } | ||
|
||
// Not possible now given argument is now a pointer to T | ||
// func TestSetterNew_IfEmptyNonPtrPanic(t *testing.T) { | ||
// defer func() { | ||
// if r := recover(); r != nil { | ||
// assert.Equal(t, "setter.SetDefault: Expected first argument to be of type reflect.Ptr", r) | ||
// } | ||
// }() | ||
|
||
// var thing string | ||
// // Should panic | ||
// setter.Default(thing, "thrawn") | ||
// assert.Fail(t, "Should have caught panic") | ||
// } |
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.
should it be exportable?
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 question. The original method is both exported and used in a few libraries so that was the motivation, though I'm still pondering if it's needed 🤔
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.
It is better to make it non-exportable and who needs could copy it as it's pretty small:
https://go-proverbs.github.io/
Well, I just don't like this name 😄
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.
Bahaha I don't like it either :D I'll leave the existing method (IsZero) exported and un-export this one, given the restricted use 👍