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.

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(setup): add missing "Generate" equivalency #173

Merged
merged 9 commits into from
Nov 25, 2024

Conversation

rebornplusplus
Copy link
Member

yamlPath.SameContent() seems to be missing checks for "Generate" equivalency. This PR adds that back.

``yamlPath.SameContent()`` seems to be missing checking for "Generate"
equivalency. This commit adds that back.
@rebornplusplus rebornplusplus marked this pull request as ready for review November 8, 2024 12:29
Copy link
Collaborator

@letFunny letFunny left a comment

Choose a reason for hiding this comment

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

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.

@rebornplusplus
Copy link
Member Author

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 letFunny added Polish Refactorings, etc Simple Nice for a quick look on a minute or two labels Nov 22, 2024
Copy link
Contributor

@niemeyer niemeyer left a comment

Choose a reason for hiding this comment

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

Should this have a test? A patch with zero effect and zero tests smells strange.

@rebornplusplus
Copy link
Member Author

Should this have a test? A patch with zero effect and zero tests smells strange.

Thanks for pointing it out. I have added some test to check yamlPath.SameContent() in 4c67e31.

Copy link
Collaborator

@letFunny letFunny left a comment

Choose a reason for hiding this comment

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

Thanks Rafid, added a couple of comments.

internal/setup/setup_test.go Outdated Show resolved Hide resolved
internal/setup/setup_test.go Outdated Show resolved Hide resolved
internal/setup/setup_test.go Show resolved Hide resolved
rebornplusplus and others added 4 commits November 25, 2024 20:33
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.
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.
Copy link
Contributor

@niemeyer niemeyer left a comment

Choose a reason for hiding this comment

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

Thanks for the testing.

@niemeyer niemeyer merged commit e2ee603 into canonical:main Nov 25, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Polish Refactorings, etc Simple Nice for a quick look on a minute or two
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants