Skip to content

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

Merged
yxxhero merged 1 commit into
databus23:masterfrom
TheMeier:empty_diff
May 6, 2025
Merged

feat: change output if all diff output is suppressed#768
yxxhero merged 1 commit into
databus23:masterfrom
TheMeier:empty_diff

Conversation

@TheMeier

Copy link
Copy Markdown
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

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

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.

Comment thread diff/report.go Outdated
Comment thread diff/report.go Outdated
@yxxhero

yxxhero commented Apr 19, 2025

Copy link
Copy Markdown
Collaborator

did you test it locally?

Comment thread diff/diff.go Outdated
@TheMeier

Copy link
Copy Markdown
Contributor Author

did you test it locally?

I did add a unit test.

@yxxhero

yxxhero commented Apr 20, 2025

Copy link
Copy Markdown
Collaborator
		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

yxxhero commented Apr 20, 2025

Copy link
Copy Markdown
Collaborator

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

TheMeier commented Apr 20, 2025

Copy link
Copy Markdown
Contributor Author

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

yxxhero commented Apr 20, 2025

Copy link
Copy Markdown
Collaborator

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
Copy Markdown
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

@yxxhero yxxhero left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM

@yxxhero

yxxhero commented Apr 22, 2025

Copy link
Copy Markdown
Collaborator

@mumoshu WDYT?

@yxxhero yxxhero merged commit b0d2e61 into databus23:master May 6, 2025
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.

3 participants