refactor: expose the DefaultReporter

[sh0rez]

I am trying to stop go-cmp from calling fmt.Stringer, as this in some
cases makes the output worse instead of better.

The DefaultReporter implementation is quite customizable, but this
functionality is not exposed to the user.

While users can register custom exporters, they would then need to
re-implement the entire DefaultExporter, even though changing a single
bool in it's config would suffice.

This PR exposes the DefaultReporter implementation so it can be
customized and used outside of this package

[dsnet]

The philosophical position of this project is to give as little knobs as possible to tweak the output. If the output is subpar (e.g., calling String is unhelpful), it is better to understand why and adjust the heuristics in the reporter to do a better job. Can you provide some examples where the String method has worse output than otherwise?

[sh0rez]

Sure! I have a type that uses yaml.Marshal as it's string function.

Because this is called, the diff now contains yaml squished to a single line, where I can't really see what's going on:

        process_test.go:95:   manifest.List{
            - 	s"apiVersion: v1\nkind: Service\nmetadata:\n  name: grafana\nspec:\n  ports:\n  - name: grafana\n    port: 3000\n    targetPort: 3000\n  selector:\n    app: grafana\n",
            - 	s"apiVersion: apps/v1\nkind: Deployment\nmetadata:\n  name: grafana\nspec:\n  replicas: 1\n  template:\n    containers:\n    - image: grafana/grafana\n      name: grafana\n    metadata:\n      labels:\n        app: grafana\n",
            + 	s"apiVersion: v1\nitems:\n- apiVersion: apps/v1\n  kind: Deployment\n  metadata:\n    name: grafana\n  spec:\n    replicas: 1\n    template:\n      containers:\n      - image: grafana/grafana\n        name: grafana\n      metadata:\n        labels:\n          app: grafana"...,
              }

With regular Go-style printing however, this would be perfectly usable:

        process_test.go:95:   manifest.List{
            - 	{
            - 		"apiVersion": string("v1"),
            - 		"kind":       string("Service"),
            - 		"metadata":   map[string]interface{}{"name": string("grafana")},
            - 		"spec": map[string]interface{}{
            - 			"ports":    []interface{}{map[string]interface{}{...}},
            - 			"selector": map[string]interface{}{"app": string("grafana")},
            - 		},
            - 	},
            - 	{
            - 		"apiVersion": string("apps/v1"),
            - 		"kind":       string("Deployment"),
            - 		"metadata":   map[string]interface{}{"name": string("grafana")},
            - 		"spec": map[string]interface{}{
            - 			"replicas": float64(1),
            - 			"template": map[string]interface{}{"containers": []interface{}{...}, "metadata": map[string]interface{}{...}},
            - 		},
            - 	},
            + 	{
            + 		"apiVersion": string("v1"),
            + 		"items": []interface{}{
            + 			map[string]interface{}{
            + 				"apiVersion": string("apps/v1"),
            + 				"kind":       string("Deployment"),
            + 				"metadata":   map[string]interface{}{...},
            + 				"spec":       map[string]interface{}{...},
            + 			},
            + 			map[string]interface{}{
            + 				"apiVersion": string("v1"),
            + 				"kind":       string("Service"),
            + 				"metadata":   map[string]interface{}{...},
            + 				"spec":       map[string]interface{}{...},
            + 			},
            + 		},
            + 		"kind": string("List"),
            + 	},
              }

[dsnet]

Thanks for the example, can you also show what it would look like if it had used the regular Go-style printing (for comparison)?

[sh0rez]

Sure, sorry for that. Update above comment!

[dsnet]

Thanks for the examples. I'm inclined to say that we should adjust the reporter to print multiline String output as multiline output (rather than a quoted string) if this is this for a added, removed, or modified element (i.e., we continue to use quoted string if the element is equal).

As a straw-man proposal, it might look something like:

  manifest.List{
- 	(
- 		s"""
- 		apiVersion: v1
- 		kind: Service
- 		metadata:
- 		  name: grafana
- 		spec:
- 		  ports:
- 		  - name: grafana
- 		    port: 3000
- 		    targetPort: 3000
- 		  selector:
- 		    app: grafana
- 		"""
- 	),
- 	(
- 		s"""
- 		apiVersion: apps/v1
- 		kind: Deployment
- 		metadata:
- 		  name: grafana
- 		spec:
- 		  replicas: 1
- 		  template:
- 		    containers:
- 		    - image: grafana/grafana
- 		      name: grafana
- 		    metadata:
- 		      labels:
- 		        app: grafana
- 		""",
- 	),
+ 	(
+ 		s"""
+ 		apiVersion: v1
+ 		items:
+ 		- apiVersion: apps/v1
+ 		  kind: Deployment
+ 		  metadata:
+ 		    name: grafana
+ 		  spec:
+ 		    replicas: 1
+ 		    template:
+ 		      containers:
+ 		      - image: grafana/grafana
+ 		        name: grafana
+ 		      metadata:
+ 		        labels:
+ 		          app: grafana
+ 		... // 5 additional lines
+ 		"""
+ 	),
  }

[sh0rez]

For this specific case, that would indeed solve the problem in an elegant way 👍

In the general picture, I'd be a bit concerned about the Stringer use, because if I messed up my Stringer implementation to swallow data (ie. I could just omit a struct field), Diff would not detect that anymore and my test might pass while actually being invalid.

[dsnet]

For this specific case, that would indeed solve the problem in an elegant way +1

Can you file an issue for this? Alternatively, you're welcome to try implementing this yourself. The logic for triple-quoted strings in a diff is here, you may need to refactor parts of it out so that you can share similar functionality from the logic to print the stringer.

if I messed up my Stringer implementation to swallow data (ie. I could just omit a struct field), Diff would not detect that anymore and my test might pass while actually being invalid.

The reporter has logic to continually printing with more verbosity if the printed output is identical between the added and removed element. Thus, if the String method left out information that was the semantic reason for a difference, the reporter should eventually figure out that the String method alone doesn't provide that information and fall back to formatting the values as pseudo-Go objects.

As much as possible, we'd like to push any notion of knobs for the reporter to be reasonable heuristics in the reporter such that it does a decent job a majority of the time, so that users don't need to play the game of guessing the best set of reporter knobs to format the diff output.

[dsnet]

#229 formats multiline strings within inequal nodes using the triple-quote syntax.

[sh0rez]

That's so cool! Imo that's sufficient, we can close this once merged :)

[dsnet]

Closing in favor of #229