-
Notifications
You must be signed in to change notification settings - Fork 402
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 option to omit empty/nil variadic slice when unroll-variadic
is false
#624
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1566,6 +1566,34 @@ func (s *GeneratorSuite) TestGeneratorVariadicArgsAsOneArg() { | |
) | ||
} | ||
|
||
func (s *GeneratorSuite) TestGeneratorVariadicArgsOmitEmpty() { | ||
expectedBytes, err := os.ReadFile(getMocksPath("RequesterVariadicOmitEmpty.go")) | ||
s.NoError(err) | ||
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 should be |
||
expected := string(expectedBytes) | ||
expected = expected[strings.Index(expected, "// RequesterVariadicOmitEmpty is"):] | ||
generator := NewGenerator( | ||
s.ctx, GeneratorConfig{ | ||
StructName: "RequesterVariadicOmitEmpty", | ||
InPackage: true, | ||
UnrollVariadic: false, | ||
OmitEmptyRolledVariadic: true, | ||
}, s.getInterfaceFromFile("requester_variadic.go", "RequesterVariadic"), pkg, | ||
) | ||
s.NoError(generator.Generate(s.ctx), "The generator ran without errors.") | ||
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. The message to |
||
|
||
var actual []byte | ||
actual, fmtErr := format.Source(generator.buf.Bytes()) | ||
s.NoError(fmtErr, "The formatter ran without errors.") | ||
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. ditto |
||
|
||
expectedLines := strings.Split(expected, "\n") | ||
actualLines := strings.Split(string(actual), "\n") | ||
|
||
s.Equal( | ||
expectedLines, actualLines, | ||
"The generator produced unexpected output.", | ||
) | ||
} | ||
|
||
func TestRequesterVariadicOneArgument(t *testing.T) { | ||
t.Run("Get \"1\", \"2\", \"3\"", func(t *testing.T) { | ||
m := mocks.RequesterVariadicOneArgument{} | ||
|
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.
Not super happy with the naming here.
Ideally
unroll-variadic
would be calledroll-variadic
and defaulted to false. We could then call this new optionroll-variadic-omit-empty
which feels better.Curious - how do we feel about deprecating
unroll-variadic
in place ofroll-variadic
?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.
Deprecating the variable name is probably a lot of effort. Technically speaking, "unroll" makes more sense as it is "expanding"/"unrolling" the slice into the variadic
mock.On(
testify call.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.
A part of me also feels like, if we do add this feature, it doesn’t need to be behind a feature flag. I can’t think of a case where this would break someone relying on the current logic?