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

Add @immutable annotation to Equatable class #25

Closed
basketball-ico opened this issue Jul 12, 2019 · 10 comments
Closed

Add @immutable annotation to Equatable class #25

basketball-ico opened this issue Jul 12, 2019 · 10 comments
Assignees
Labels
Projects

Comments

@basketball-ico
Copy link

@basketball-ico basketball-ico commented Jul 12, 2019

Is your feature request related to a problem? Please describe.

At the moment Equatable only work for immutable classes, but you can extends Equatable in a classes that not are immutable

#21
#22

Describe the solution you'd like
You can add the @immutable annotation, and now you get analysis warning.

@immutable
class Equatable ....

Describe alternatives you've considered
Equatable can override the hashCode and == for not immutable objects.

Additional
Also like the same behavior for EquatableMixin but i don't know how to do that.

@felangel felangel self-assigned this Jul 13, 2019
@felangel

This comment has been minimized.

Copy link
Owner

@felangel felangel commented Jul 13, 2019

Great idea 💯

I'll make the change shortly 👍

@felangel felangel mentioned this issue Jul 13, 2019
3 of 3 tasks complete
@felangel felangel added this to In progress in equatable Jul 13, 2019
@felangel

This comment has been minimized.

Copy link
Owner

@felangel felangel commented Jul 15, 2019

Published in v0.3.0

@felangel felangel closed this Jul 15, 2019
@felangel felangel moved this from In progress to Done in equatable Jul 15, 2019
@dmh2000

This comment has been minimized.

Copy link

@dmh2000 dmh2000 commented Jul 25, 2019

does the class being extended from equatable really need to be fully immutable, or is it only the fields that are used in super([props]) as the comparators that need to be final? in the second case, the @immutable annotation causes a warning that is not be needed. if indeed all fields in the class must be immutable then the annotation is right. (I have a class with a few properties but only a couple are needed to verify equality)

@felangel

This comment has been minimized.

Copy link
Owner

@felangel felangel commented Jul 25, 2019

@dmh2000 could you provide a bit more info regarding why you only use a subset of the object's properties for equality comparison? Technically only the properties passed to the super class must be final.

@dmh2000

This comment has been minimized.

Copy link

@dmh2000 dmh2000 commented Jul 25, 2019

@felangel

this is kind of confusing but I hope this explanation is clear.

I am using flutter_bloc and the bloc has a list of objects (that extend equatable) in it. those objects are fetched one-by-one when the bloc is initialized. but one field in each object can't be set until all the objects are fetched because it depends on the content of the other objects. Its just an int and it isn't needed in the equality comparison. it has to do with what is in the JSON i receive and I don't have control of that.

So so right now I fetch the objects one by one, construct them, add them to the list, then make a pass through the list to update that one field. that prevents me from making that one field final. so i get the 'not immutable' warning.

I can refactor my code to use a temporary list that captures the data and updates the field. then creates the final list of immutable objects. this would get rid of the warning. but as you say if its technically ok to leave that field mutable then I will just leave it as is and ignore the warning.

@biklas7

This comment has been minimized.

Copy link

@biklas7 biklas7 commented Aug 6, 2019

@felangel What about if we want to Mock a class that extends Equatable? I get warnings in almost all my tests now.

@felangel

This comment has been minimized.

Copy link
Owner

@felangel felangel commented Aug 8, 2019

Hi @biklas7 👋
Can you please provide an example of a case when you'd need to mock a class that extends Equatable? I'm happy to revert this change if it's causing more harm than good but ideally classes that extend Equatable should be simple models which aren't typically mocked. Thanks 👍

@biklas7

This comment has been minimized.

Copy link

@biklas7 biklas7 commented Aug 8, 2019

Hello, @felangel thanks for the reply. Here's my simple example:

I have the User class which extends equatable. It's a simple class with fields like id, name... Then I have a LoginBloc to handle the form validation and login logic.

For that bloc I have this test:

class MockUserRepository extends Mock implements UserRepository {}

// ignore: must_be_immutable
class MockUser extends Mock implements User {}

....

    test('Emits [Loading, LoginSuccess] for valid input', () {
      final List<LoginState> expectedResponses = [
        EmptyForm(),
        Loading(),
        LoginSuccess(),
      ];

      when(userRepository.login(
        email: validEmail,
        password: validPassword,
      )).thenAnswer((_) => Future.value(user));

      expectLater((loginBloc.state), emitsInOrder(expectedResponses));

      loginBloc.dispatch(Login(
        email: validEmail,
        password: validPassword,
      ));
    });

I'm using the mocked User to mock the response to the login future in my repository.

I'm pretty new to Flutter tests, maybe I shouldn't do things like this. What do you think?

@biklas7

This comment has been minimized.

Copy link

@biklas7 biklas7 commented Aug 8, 2019

@felangel let me just add this to my comment. Maybe it's not you that needs to revert this change, maybe it's the mockito package that needs to be updated.

Please refer to this issue on the mockito package.

@felangel

This comment has been minimized.

Copy link
Owner

@felangel felangel commented Aug 10, 2019

@biklas7 that's great! I think the idea solution would be for mockito to address this so it's good to hear that it's in the works 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
equatable
  
Done
4 participants
You can’t perform that action at this time.