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.

Sign up for GitHub

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

Fix typo in merge2 #5489

Merged
merged 1 commit into from
Dec 21, 2023
Merged

Fix typo in merge2 #5489

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion kyaml/yaml/merge2/merge2.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,9 +45,9 @@ func MergeStrings(srcStr, destStr string, infer bool, mergeOptions yaml.MergeOpt
}

type Merger struct {
// for forwards compatibility when new functions are added to the interface
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel this comment means that when we need to add some field in this struct.
So, I think this comment is still in the right place.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @koba1t , and thanks for your review! I think I disagree with you on your point:
As far as I understand, the comment is applying to the pattern that is used to ensure that if functions are added to the walk.Visitor interface, the struct will still implement all of them (if not, the build will failed because of the line var _ walk.Visitor = Merger{}).
And also the comment refers to the interface, which Merger is not (it is a struct), so again, the comment should apply to the line mentioned above.
So if a new function is added on the walk.Visitor interface, the line var _ walk.Visitor = Merger{} will ensure that the Merger struct is still (forward-)compatible with the interface, which is basically what the comment is saying.
WDYT?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, Thanks for describing this code. I misunderstood walk.Visitor as not an interface.
I agree with what you are doing now.

}

// for forwards compatibility when new functions are added to the interface
var _ walk.Visitor = Merger{}

func (m Merger) VisitMap(nodes walk.Sources, s *openapi.ResourceSchema) (*yaml.RNode, error) {
Expand Down