Skip to content

feat: change output if all diff output is suppressed #768

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

TheMeier
Copy link
Contributor

If a diff exists but is suppressed in the output change the text to make it more clear what is actually happening.

@yxxhero yxxhero requested a review from Copilot April 19, 2025 10:48
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR introduces a new change style ("MODIFY_SUPPRESSED") to improve the clarity of diff output when all changes are suppressed. It updates the report generation across various output formats and tests for the new suppressed diff output.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
diff/report.go Adds a new ChangeStyle for "MODIFY_SUPPRESSED" with a descriptive message in diff and simple reports. Updates JSON and template reports with an empty message.
diff/diff_test.go Updates tests to expect the new "MODIFY_SUPPRESSED" message when diffs are suppressed.
diff/diff.go Modifies diff suppression logic to assign the "MODIFY_SUPPRESSED" change type when applicable.

@yxxhero
Copy link
Collaborator

yxxhero commented Apr 19, 2025

did you test it locally?

diff/diff.go Outdated
filteredReport.addEntry(entry.key, entry.suppressedKinds, entry.kind, entry.context, []difflib.DiffRecord{}, "MODIFY_SUPPRESSED")
} else {
filteredReport.addEntry(entry.key, entry.suppressedKinds, entry.kind, entry.context, []difflib.DiffRecord{}, entry.changeType)
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

so many if? what can we optimize?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well there are 3 cases here, how could you satisfy that with less then 2 ifs?

We rewrite it a bit:

		if !containsDiff && entry.changeType == "MODIFY" {
			entry.changeType = "MODIFY_SUPPRESSED"
		}
		if containsDiff {
			filteredReport.addEntry(entry.key, entry.suppressedKinds, entry.kind, entry.context, diffs, entry.changeType)
		} else {
  		filteredReport.addEntry(entry.key, entry.suppressedKinds, entry.kind, entry.context, []difflib.DiffRecord{}, entry.changeType)
		}

or

		if containsDiff {
			filteredReport.addEntry(entry.key, entry.suppressedKinds, entry.kind, entry.context, diffs, entry.changeType)
		} else {
		  if entry.changeType == "MODIFY" {
				entry.changeType = "MODIFY_SUPPRESSED"
			}
  		filteredReport.addEntry(entry.key, entry.suppressedKinds, entry.kind, entry.context, []difflib.DiffRecord{}, entry.changeType)
		}

@TheMeier
Copy link
Contributor Author

did you test it locally?

I did add a unit test.

@yxxhero
Copy link
Collaborator

yxxhero commented Apr 20, 2025

		if !containsDiff && entry.changeType == "MODIFY" {
			entry.changeType = "MODIFY_SUPPRESSED"
		}
		if containsDiff {
			filteredReport.addEntry(entry.key, entry.suppressedKinds, entry.kind, entry.context, diffs, entry.changeType)
		} else {
  		filteredReport.addEntry(entry.key, entry.suppressedKinds, entry.kind, entry.context, []difflib.DiffRecord{}, entry.changeType)
		}

this is better.

@yxxhero
Copy link
Collaborator

yxxhero commented Apr 20, 2025

or we can use new var?

@TheMeier TheMeier force-pushed the empty_diff branch 2 times, most recently from bd45ab8 to 8883fb5 Compare April 20, 2025 11:49
@TheMeier
Copy link
Contributor Author

TheMeier commented Apr 20, 2025

I have changed the code.
I don't see how/where a new variable would change anything.

How about this variant:

		var diffRecords []difflib.DiffRecord
		switch {
		case containsDiff:
			diffRecords = diffs
		case entry.changeType == "MODIFY":
			entry.changeType = "MODIFY_SUPPRESSED"
			diffRecords = []difflib.DiffRecord{}
		default:
			diffRecords = []difflib.DiffRecord{}
		}

		filteredReport.addEntry(entry.key, entry.suppressedKinds, entry.kind, entry.context, diffRecords, entry.changeType)

@yxxhero
Copy link
Collaborator

yxxhero commented Apr 20, 2025

I have changed the code. I don't see how/where a new variable would change anything.

How about this variant:

		var diffRecords []difflib.DiffRecord
		switch {
		case containsDiff:
			diffRecords = diffs
		case entry.changeType == "MODIFY":
			entry.changeType = "MODIFY_SUPPRESSED"
			diffRecords = []difflib.DiffRecord{}
		default:
			diffRecords = []difflib.DiffRecord{}
		}

		filteredReport.addEntry(entry.key, entry.suppressedKinds, entry.kind, entry.context, diffRecords, entry.changeType)

we can try this.

@TheMeier
Copy link
Contributor Author

@yxxhero I have simplified the code even more using switch/case. Also I fixed the the color to be actually used and chose a color that actually exists in the lib.
unit tests are ok for me, did some manual tests which also are ok for me. So in my opinion this is now good to go

Copy link
Collaborator

@yxxhero yxxhero left a comment

Choose a reason for hiding this comment

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

LGTM

@yxxhero
Copy link
Collaborator

yxxhero commented Apr 22, 2025

@mumoshu WDYT?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants