Review Comments

[rickb777]

Firstly, I like the API. What you're trying to achieve and the way you've applied generics is very commendable.

It got me interested in delving deeper into the implementation. I came up with a few issues, which I'm going to list here rather than making a Github Issue for each. (We could do that later. Maybe PRs too.)

I don't want this to be too picky but it's hopefully going to help make Gocrest better for everyone. So in no particular order:

1. Containment semantics - especially is/contains.go func listContains

func listContains doesn't implement what the documentation says. Maybe the documentation is wrong. The function checks whether all elements are present in the actual list, regardless of order. There need to be functions to cover both the unordered and ordered cases.

Note that is.ArrayMatching has the same documentation issue., Also, has/haseveryelement.go func EveryElement is confused with is.ArrayMatching.

To make things as simple as possible for users, I suggest using the is package for ordered comparisons of arrays and slices; then using the has package for unordered containment assertions. This will mean changing the documentation in some places and implementation in others.

For map assertions, the functions in the is package should verify all keys or values are present and no others are. Those in the has package should do a simpler subset test.

2. is/equalto.go func EqualTo

Google's developers have a cmp package. It's a really heavy to use but it highlights something missing from Gocrest: comparing value equality sometimes requires refinement. The obvious cases are when comparing floats given a margin of error, or when comparing types such as time.Time that provide a Equal method. Gocrest doesn't provide for this yet.

3. is/equalto.go func EqualTo

Also related to the point above, the test error message printed for structs that have different values are unhelpfully terse. It would be good to indicate what part within the struct (or array, map, etc) was different. For comparison, see Juju testing/checkers/deepequal.go - but note the License is incompatible with BSD. It's not ideal to add an external dependency on that package and its API isn't great, so some fresh implementation along similar lines would be needed.

4. Angle brackets

Mostly, actual and expected values are rendered like fmt.Sprintf("actual <%v>", actual), but this looks cluttered in practice. IMO, It would help the user if Gocrest switched to fmt.Sprintf("actual %+v", actual) style throughout. This is in quite a lot of places but relatively easy to correct.

5. is.Nil() is confusing

This is declared in is/nil.go. It caught me out as soon as I started using Gocrest. What it means is is.NoError() so it ought to be called that. Other irrelevant things can be nil, hence the confusion. The existence of is.NilPtr() is fine for the obvious pointer case.

6. Redundant code

Just a few things I spotted: in is/nil.go, the assignments to the match.Actual field are all redundant - see then/assertthat.go which provides the default.

[corbym]

Thank you for your kind words and your detailed analysis.

I will look into each of your points and make the appropriate changes/issues as required. Go is not my native language, so I appreciate the guidance.

Feel free, however, to open your own prs as you feel fit in the meantime!

[corbym]

Hi,
Again, thanks for your thorough analysis of the lib. I've put together some responses to each numbered point:

1. Yes, the documentation is incorrect. I will correct the it. Moving things around in packages will have to wait, unless you wish to create a PR yourself? However the package structure here is simply an aid to natural language, so we would need to be careful that the changes made sense to be "spoken out loud" so to speak.
2. Sure - I get that. A custom matcher can be created to deal with Equals interfaces, I expect. Given that an equals method does not seem to be the standard interface for comparison, I'm not sure about making a specific is.EqualTo method for something like this part of the lib though.
3. I had considered this, however the effort required to work out the differences seemed to outweight its benefit (at least at the time I wrote this lib). Feel free to raise a PR for this, it would be interesting to see how it was approached.
4. The idea of angled brackets is a) to mimic the hamcrest libraries out put to some degree (which does similar) and b) to notify the user if there are whitespaces or other at the end of assertion output that might otherwise be missed in comparison. As you can see, BaseDescription tries to understand the type being asserted in order to build the message, something that GoCrest could do, but it seemed a waste of effort given that the idea is just to isolate the actual output from the rest of the message.
5. It seemed to me that there was no real way to identify what Nil really is, and that sure, it could mainly be used to do error checking. However I wasn't certain enough to say that "NoError" would be well understood, and perhaps checking Nil other things might be useful, but I am not opposed to the rename if we make sure its documented and the major version changed or something.
6. I will fix this, thanks for pointing it out.

Again, thank you for all the detailed input. Very much appreciated!