go-test / deep

Golang deep variable equality test that returns human-readable differences
MIT License
743 stars 54 forks source link

Allow Equal() to be used by different packages in one program. #20

Open gcla opened 6 years ago

gcla commented 6 years ago

Some packages may require Equal()'s parameters to be set in a particular way that is incompatible with other users within the same program. The global configuration parameters can be changed and restored, but this could lead to bugs due to race conditions. This commit makes the parameters that control Equal()'s operation part of a structure, Comparer, for which Equal() is now a method. Users can configure their own Comparer struct if desired. To preserve the existing package interface, the package-level Equals() method will use a default Comparer object that relies on pointers to the current global configuration parameters (pointers so that the operation of the global Equals() function will change immediately upon changing the value of any global configuration setting).

coveralls commented 6 years ago

Coverage Status

Coverage remained the same at 100.0% when pulling 1985ad23777297fb09db0338a8993a89920b4ced on gcla:master into 57af0be209c537ba1d9c2a4b285ab7aea0897e51 on go-test:master.

daniel-nichter commented 4 years ago

It's a good idea and would make a good change for v1.1. There's a couple things I'd change wrt the design, etc.

  1. s/MakeComparer/New Comparer/ as "NewX" is more idiomatic Go
  2. Struct fields don't need to be pointers, i.e. FloatPrecision int is sufficient. Out of curiosity, why are they pointers?
  3. Keeping Equal(a, b interface{}) []string is good, really want to do this. I'd simplify and hide the default:
    var defaultComparer = Comparer{
    // default values
    }

Might have more feedback after seeing changes, if you decided to make them (and merge latest to fix the merge conflicts).

gcla commented 4 years ago

Hi - thanks for considering the PR :-)

I'll update the branch so it's based off of latest and resubmit. I'll rename MakeComparer to NewComparer, and hide the default, as you suggest.

For point (2) - it seemed to me that the current implementation made FloatPrecision, MaxDepth, etc a dynamic part of the interface in that each call to Equal() would use their current values in computing the result. For example, if the user ran

deep.MaxDepth = 5
Equal(a, b)

the result might be different from

deep.MaxDepth = 10
Equal(a, b)

To preserve that behavior, I made the Comparer struct use pointers for these fields, and by default I made the pointers point to these exported variables. So even though, with this PR, Equal() defers to defaultComparer.Equal(), these package-level variables continue to have the same effect on the computation.

I'll definitely simplify things if you think that's the best way to go.

flga commented 4 years ago

Sorry for jumping in, but I was also toying around with this idea for fun. How about something like this? It's backwards compatible without introducing new public types (besides the variadic options). I have this implemented if you're interested.

deep.CompareUnexportedFields = false
deep.FloatPrecision = 1

s1 := s{a: 0.0078125}  // 1/128
s2 := s{a: 0.00390625} // 1/256

deep.Equal(s1, s2) // nil
deep.Equal(s1, s2, deep.WithCompareUnexportedFields()) // nil
deep.Equal(s1, s2, deep.WithCompareUnexportedFields(), deep.WithFloatPrecision(3)) // "a: 0.0078125 != 0.00390625"
gcla commented 4 years ago

Hi @flga - your patch would definitely work for my needs, and I'd be happy to just switch over to your implementation :-)