stretchr / testify

A toolkit with common assertions and mocks that plays nicely with the standard library
MIT License
22.5k stars 1.56k forks source link

assert: make *AssertionFunc types just aliases #1563

Open dolmen opened 4 months ago

dolmen commented 4 months ago

Summary

We don't need to define "hard" types for *AssertionFunc types as we don't attach methods to them. Also, making them just type aliases will give more flexibility to users of our API. So ComparisonAssertionFunc and other *AssertionFunc types are now just type aliases.

Changes

Make ComparisonAssertionFunc, ValueAssertionFunc, BoolAssertionFunc and ErrorAssertionFunc just type aliases instead of "hard" types.

Motivation

Related issues

brackendawson commented 4 months ago

LGTM. The only concern is, is this a backward compatible change? I believe type assertions work differently with type aliases and type definitions?

I believe the only observable change is the type name. I can fabricate code that would be impacted by this change:

func TestIfy(t *testing.T) {
    var f assert.ErrorAssertionFunc
    assert.Equal(t, reflect.TypeOf(f).String(), "assert.ErrorAssertionFunc")
}

But it's extraordinarily unlikely anyone relies on that. Whether this is a breaking change is up to our opinion and I have seen Go make more noticeable changes in the standard library (the new optional Unwrap() []error method on errors for example).

I would make this change if it were necessary, since it's not strictly necessary for us I would personally not make this change in v1. I'm not going to block the PR though, I will leave the ultimate decision up to consensus of the maintainers.

arjunmahishi commented 4 months ago

I would make this change if it were necessary, since it's not strictly necessary for us I would personally not make this change in v1. I'm not going to block the PR though, I will leave the ultimate decision up to consensus of the maintainers.

I agree with this. This is not strictly necessary and I would skip this too.