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

assert: make *AssertionFunc types just aliases #1563

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

Conversation

dolmen
Copy link
Collaborator

@dolmen dolmen commented Mar 4, 2024

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

  • More flexibility for API users
  • Less resource usage at runtime (type aliases are used only for compile step) (yes, negligible difference)

Related issues

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.
@dolmen dolmen self-assigned this Mar 5, 2024
@dolmen dolmen added pkg-assert Change related to package testify/assert pkg-require Change related to package testify/require labels Mar 5, 2024
Copy link
Collaborator

@arjunmahishi arjunmahishi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@brackendawson
Copy link
Collaborator

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

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg-assert Change related to package testify/assert pkg-require Change related to package testify/require
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants