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

Implement toString feature #45

Merged
merged 6 commits into from
Feb 9, 2020
Merged

Implement toString feature #45

merged 6 commits into from
Feb 9, 2020

Conversation

nfdz
Copy link
Contributor

@nfdz nfdz commented Dec 9, 2019

Status

READY

Breaking Changes

NO

Description

I implemented the toString method feature using the props.

I think it is a very useful feature for testing and debugging that most of developers that use this library would like to use. It could be able to do this through an own subclass of equatable or another library but I think that it is overkill for this small feature. This library override already that method and there is no impact implementing this.

Related PRs

No related PRs.

Todos

There is no TODOs

  • Tests
  • Documentation
  • Examples

Impact to Remaining Code Base

This new feature will not have impact in the code base, functionality or performance.
It is disabled by default and does not force the user to do changes.

@nfdz nfdz requested a review from felangel as a code owner December 9, 2019 15:56
@codecov-io
Copy link

codecov-io commented Dec 9, 2019

Codecov Report

Merging #45 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master    #45   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           3      3           
  Lines          45     45           
=====================================
  Hits           45     45
Impacted Files Coverage Δ
lib/src/equatable_utils.dart 100% <100%> (ø) ⬆️
lib/src/equatable_mixin.dart 100% <100%> (ø) ⬆️
lib/src/equatable.dart 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bb1723b...b0e4a3f. Read the comment docs.

@winterDroid
Copy link

Would it be possible to provide also the names for the props to make the toString() more readable?

@nfdz
Copy link
Contributor Author

nfdz commented Feb 4, 2020

Would it be possible to provide also the names for the props to make the toString() more readable?

We do not have the name of the props. We could do a workaround and provide these labels if we see it necessary.
However, looks like this feature (the whole toString feature) is not interesting for the users and codeowners of this project, so probably it is not going to be merged. @winterDroid @felangel

@felangel
Copy link
Owner

felangel commented Feb 4, 2020

Hey @nfdz! Sorry for not having taken a look at this PR. I will review it shortly 👍

@jaripekkala
Copy link

I would love to see the prop names as well. Maybe something like this

mixin NamedProps on Equatable {
  @override
  get props => this.namedProps.values.toList();
  
  Map<String, Object> get namedProps;
  
  @override
  toString() => '$runtimeType$namedProps';
}

Usage

class User extends Equatable with NamedProps {
  final String name;
  final int age;
  
  @override
  get namedProps => {'name': name, 'age': age};
  
  User(this.name, this.age);
}
print(User('Foo', 24)); // User{name: Foo, age: 24}

example/main.dart Outdated Show resolved Hide resolved
@nfdz
Copy link
Contributor Author

nfdz commented Feb 9, 2020

@felangel Thank you very much for the feedback. I have implemented all the suggestions.

About this:

Might want to have a check for if is props is null.

My first implementation was with this check. However, I saw that the hashCode method failed in the tests that I wrote to test null props, so I thought we had the assumption that props would never be null.

I rewrite the code checking nulls, the tests and also fix the hashCode method.

@nfdz
Copy link
Contributor Author

nfdz commented Feb 9, 2020

I would love to see the prop names as well. Maybe something like this

mixin NamedProps on Equatable {
  @override
  get props => this.namedProps.values.toList();
  
  Map<String, Object> get namedProps;
  
  @override
  toString() => '$runtimeType$namedProps';
}

Usage

class User extends Equatable with NamedProps {
  final String name;
  final int age;
  
  @override
  get namedProps => {'name': name, 'age': age};
  
  User(this.name, this.age);
}
print(User('Foo', 24)); // User{name: Foo, age: 24}

@felangel do you have thoughts about this? Maybe we should study this in another PR.

Copy link
Owner

@felangel felangel left a comment

Choose a reason for hiding this comment

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

LGTM 👍
Thanks so much for the contribution and sorry for the delayed PR review!

@felangel
Copy link
Owner

felangel commented Feb 9, 2020

I would love to see the prop names as well. Maybe something like this

mixin NamedProps on Equatable {
  @override
  get props => this.namedProps.values.toList();
  
  Map<String, Object> get namedProps;
  
  @override
  toString() => '$runtimeType$namedProps';
}

Usage

class User extends Equatable with NamedProps {
  final String name;
  final int age;
  
  @override
  get namedProps => {'name': name, 'age': age};
  
  User(this.name, this.age);
}
print(User('Foo', 24)); // User{name: Foo, age: 24}

@felangel do you have thoughts about this? Maybe we should study this in another PR.

Yeah I'd discuss that in a separate issue. I feel like at that point it's easier to just override toString manually haha. Would love to hear what everyone else thinks.

@felangel felangel merged commit 64b8545 into felangel:master Feb 9, 2020
@felangel felangel added the enhancement New feature or request label Feb 9, 2020
@nfdz
Copy link
Contributor Author

nfdz commented Feb 9, 2020

LGTM 👍
Thanks so much for the contribution and sorry for the delayed PR review!

No worries! Glad to help the project

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants