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

[C++] Add operator== for interfaces with an Equals() method #23110

Closed
asfimport opened this issue Oct 2, 2019 · 7 comments
Closed

[C++] Add operator== for interfaces with an Equals() method #23110

asfimport opened this issue Oct 2, 2019 · 7 comments

Comments

@asfimport
Copy link
Collaborator

A common pattern in tests is ASSERT_TRUE(schm->Equals(*other). The addition of overloaded equality operators will allow this o be written ASSERT_EQ(*schm, *other), which is more idiomatic GTEST usage and will allow more informative assertion failure messages.

Reporter: Ben Kietzman / @bkietz
Assignee: Ben Harkins / @benibus

PRs and other links:

Note: This issue was originally created as ARROW-6772. Please see the migration documentation for further details.

@asfimport
Copy link
Collaborator Author

Ben Kietzman / @bkietz:
operator== for Schemas was added by #5529

@asfimport
Copy link
Collaborator Author

Ben Kietzman / @bkietz:
Use the util::EqualityComparable mixin defined in #5621

At the same time it'd be useful to incorporate the util::ToStringOstreamable mixin (introduced in the same PR)

@asfimport
Copy link
Collaborator Author

Antoine Pitrou / @pitrou:
This still would need to be done for Field and DataType, at least.

@asfimport
Copy link
Collaborator Author

Todd Farmer / @toddfarmer:
This issue was last updated over 90 days ago, which may be an indication it is no longer being actively worked. To better reflect the current state, the issue is being unassigned. Please feel free to re-take assignment of the issue if it is being actively worked, or if you plan to start that work soon.

@asfimport
Copy link
Collaborator Author

Ben Harkins / @benibus:
I'm currently working on this one - planning on adding util::EqualityComparable to {}DataType{}, {}Field{}, and {}FieldRef{}. Should additional comparison tests be added (in addition to AssertXXXEqual) to type_test.cc or would that be considered redundant?

@asfimport
Copy link
Collaborator Author

Antoine Pitrou / @pitrou:
I think it would be nice to add at least basic tests to ensure that the operators properly redirect. No need to duplicate all tests though.

@asfimport
Copy link
Collaborator Author

Antoine Pitrou / @pitrou:
Issue resolved by pull request 14038
#14038

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

No branches or pull requests

1 participant