fix(setup): add missing "Generate" equivalency #173
Conversation
``yamlPath.SameContent()`` seems to be missing checking for "Generate" equivalency. This commit adds that back.
letFunny
left a comment
letFunny
left a comment
There was a problem hiding this comment.
Thanks Rafid for this PR! I was wondering why there are no new tests but it is because this change here is just for consistency, it does not fix any bug, right?
I see in the code that the only usage is in:
if yamlPath != nil && yamlPath.Generate != "" {
zeroPathGenerate := zeroPath
zeroPathGenerate.Generate = yamlPath.Generate // Generate is the same so it has no effect.
if !yamlPath.SameContent(&zeroPathGenerate) || yamlPath.Until != UntilNone {
...
}
...
} else if strings.ContainsAny(contPath, "*?") {
if yamlPath != nil {
if !yamlPath.SameContent(&zeroPath) { // If Generate was set we would not have entered this branch.
...
}
}
...
}See comments above, but essentially is has not effect in the code, still let's get this merged for consistency.
|
Yes, you are absolutely correct! if yamlPath != nil && yamlPath.Generate != "" {
zeroPathGenerate := zeroPath
zeroPathGenerate.Generate = yamlPath.Generate // Generate is the same so it has no effect.
if !yamlPath.SameContent(&zeroPathGenerate) || yamlPath.Until != UntilNone {
...
}
...
}This is the only true usage, in relative to the changes in this PR. And like you pointed out, it has no impact. But I would like to have this consistent so that we won't have to worry about it if we think of using it later at any other place. :) |
letFunny
added
Polish
niemeyer
left a comment
niemeyer
left a comment
There was a problem hiding this comment.
Should this have a test? A patch with zero effect and zero tests smells strange.
Changes lost in merge commit. This commit adds back the lost changes.
Thanks for pointing it out. I have added some test to check |
letFunny
left a comment
letFunny
left a comment
There was a problem hiding this comment.
Thanks Rafid, added a couple of comments.
Per offline comments, let's just have the SameContent tests for GeneratePath, because every other kind of paths are tested indirectly in slicer_test and setup_test.
This reverts commit 167a6eb.
Per offline comments, let's just have the SameContent tests for GeneratePath, because every other kind of paths are tested indirectly in slicer_test and setup_test. Similar to 167a6eb, but do not remove the table tests.
niemeyer
left a comment
niemeyer
left a comment
There was a problem hiding this comment.
Thanks for the testing.
3 participants
yamlPath.SameContent()seems to be missing checks for "Generate" equivalency. This PR adds that back.