Move object equality functions to another package for clarity
Summary
Move none assertion functions into a seperate package to make it clear that these do no do any assertions. There is a a lot of existing code out there with useless tests because people are assuming that assert.Foobar functions will do assertions.
Happy to take guidance on comments/naming etc.
Changes
Moved ObjectsAreEqual and ObjectsAreEqualValues and tests into a new sub package equals. Label existing functions as deprecated and call new functions instead. Call new methods anywhere in assert where the previous functions are called.
Motivation
Currently very confusing that these are in the assert package when they don't do any assertions at all.
change assert.ObjectsAreEqual to equal.ObjectsAreEqual etc
Related issues
Closes 1180
If the aim of this is to prevent people from thinking that ObjectsAreEqual is an assertion that they should use, then I think this new equal package should be internal:
testify/
|-- assert
| `-- assertions.go
|-- internal
| `-- equal
| `-- equal.go
|-- mock
| `-- mock.go
If the aim of this is to prevent people from thinking that
ObjectsAreEqualis an assertion that they should use, then I think this newequalpackage should be internal:testify/ |-- assert | `-- assertions.go |-- internal | `-- equal | `-- equal.go |-- mock | `-- mock.go
I was actually thinking that people would still continue to use these methods, as they are useful, they're just not immediately obvious that the bool needs to be checked to have a working test. But perhaps another name/location outside of assert makes more sense?
If the aim of this is to prevent people from thinking that
ObjectsAreEqualis an assertion that they should use, then I think this newequalpackage should be internal:testify/ |-- assert | `-- assertions.go |-- internal | `-- equal | `-- equal.go |-- mock | `-- mock.goI was actually thinking that people would still continue to use these methods, as they are useful, they're just not immediately obvious that the bool needs to be checked to have a working test. But perhaps another name/location outside of assert makes more sense?
While it's true that they might be useful to someone, I think people will still misuse them in exactly the same way. Testify is not a utility library, I think if people are looking for a comparison library then they should be looking at go-cmp instead.
Yeah that's a good point. Tbh then I'm a bit reticent about introducing this change then, ~~as I don't want to remove useful functionality even if it is currently being misused~~
Actually scratch that, I realise that people just need to be directed to use the Equal and EqualValues assertions instead. I will alter my PR now
Edit: Updated with suggested changes.
Is there any progress on this? If there are suggestions that are required, please let me know and I'll gladly make them. People are still being caught by this
Please rebase this branch on top of master, to fix gofmt issues.
Replaced by #1431.
Closing in favor of #1431.
Nice! Sorry I didn’t have time to rebase, it had been so long that there were more changes that needed more work. Glad to see the change got in anyway!