-
-
Notifications
You must be signed in to change notification settings - Fork 107
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
feat: make Equatable abstract mixin class
#161
feat: make Equatable abstract mixin class
#161
Conversation
abstract mixin class
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@felangel what are your thoughts?
repository: https://github.com/felangel/equatable | ||
issue_tracker: https://github.com/felangel/equatable/issues | ||
homepage: https://github.com/felangel/equatable | ||
documentation: https://github.com/felangel/equatable | ||
|
||
environment: | ||
sdk: ">=2.12.0 <3.0.0" | ||
sdk: ">=3.0.0 <4.0.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a breaking change, so I think we could remove EquatableMixin
all together and mark this as release v3?
@@ -18,7 +18,7 @@ import './equatable_utils.dart'; | |||
/// ``` | |||
/// {@endtemplate} | |||
@immutable | |||
abstract class Equatable { | |||
abstract mixin class Equatable { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As per my issue, I still doubt that this should be a class at all. This works perfectly fine as a mixin-only, so I see no reason as to why would anyone extend it instead of mixin it in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be quite inconvenient for existing apps if we forced them to refactor all classes to use equatable as a mixin imo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fair enough, for sake of compatibility it makes sense. In the long run though (and since it is a breaking change anyways) I don't see value in it being a class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@felangel even in a large project, this change should be easy to make, a simple search and replace can do 95% of the work, plus people can choose to update the version or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change is still a breaking change and I don't think it's worth it especially considering I'm planning to make the next major version of equatable use macros instead so you'd always just annotate your class with @Equatable()
class Person {
const Person({required this.name});
final String name;
} See also https://github.com/felangel/data_class which will be built on top of the equatable macro so you could go a few steps further: @Data()
class Person {
final String name;
} |
Closing for now based on my above comment, thanks so much for the contribution though -- I really appreciate it! |
Hey @felangel, since Dart won't have macros anymore, can we re-open this for discussion? |
Yup I’m planning to make this change in the next release of 3.0.0 (prerelease). |
Status
READY
Breaking Changes
NO
Description
Make Equatable class an abstract mixin class so that it can be used as a mixin
Related PRs
no
Todos
Steps to Test or Reproduce
Outline the steps to test or reproduce the PR here.
Impact to Remaining Code Base
This PR will affect: