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

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

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

Conversation

gcla
Copy link

@gcla gcla commented Jun 29, 2018

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).

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

coveralls commented Jun 29, 2018

Coverage Status

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

@daniel-nichter
Copy link
Member

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

gcla commented Jul 20, 2019

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

flga commented Aug 21, 2019

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

gcla commented Aug 31, 2019

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

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

Successfully merging this pull request may close these issues.

None yet

4 participants