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

fix: iterable equality comparisons #103

Merged
merged 2 commits into from Feb 8, 2021
Merged

fix: iterable equality comparisons #103

merged 2 commits into from Feb 8, 2021

Conversation

felangel
Copy link
Owner

@felangel felangel commented Feb 8, 2021

Status

READY

Breaking Changes

NO

Description

Todos

  • Tests
  • Documentation
  • Examples

@felangel felangel added the bug Something isn't working label Feb 8, 2021
@felangel felangel self-assigned this Feb 8, 2021
@felangel felangel merged commit b5a1e22 into master Feb 8, 2021
@felangel felangel deleted the fix/iterators branch February 8, 2021 05:21
@nxtSwitch
Copy link

Hi @felangel
Thanks for the prompt solution of the problem!

Can you please tell me if there is a way to solve this problem for manually overriding the ==? Thanks!

@felangel
Copy link
Owner Author

felangel commented Feb 8, 2021

Hi @nxtSwitch 👋

Can you provide a bit more context or a reproduction sample? I'm not sure I understand the use-case/question.

@felangel felangel added this to Done in equatable Feb 8, 2021
@nxtSwitch
Copy link

А similar case, but without using the equatable:

class SimpleEquatable<T> extends Equatable {
  const SimpleEquatable(this.data);

  final T data;

  @override
  List<Object> get props => [data];
}

@immutable
class IterableWithFlag extends Iterable<String> {
  final bool flag;
  final List<String> list;

  const IterableWithFlag({this.list, this.flag});

  @override
  bool operator ==(Object other) {
    return identical(this, other) ||
        other is IterableWithFlag && flag == other.flag;
  }

  @override
  Iterator<String> get iterator => list.iterator;
}
  const instanceA = IterableWithFlag(list: ['1', '2'], flag: true);
  const instanceB = IterableWithFlag(list: ['1', '2'], flag: false);

  print(const SimpleEquatable(instanceA) == const SimpleEquatable(instanceB)); // true

@felangel
Copy link
Owner Author

felangel commented Feb 8, 2021

@nxtSwitch I believe the reason why that isn't working is because two Iterable instances are considered equal if they have the same elements in the same order. I wouldn't recommend having a custom Iterable with a flag. Instead it would be good to decouple the two:

@immutable
class Iterable extends Iterable<String> {
  final List<String> list;

  const IterableWithFlag({this.list});

  @override
  Iterator<String> get iterator => list.iterator;
}

class FlagWrapper<T> {
  final T data;
  final bool flag;

  const FlagWrapper({this.data, this.flag});

  @override
  bool operator ==(Object other) {
    return identical(this, other) ||
        other is FlagWrapper<T> && flag == other.flag && data == other.data;
  }
}

Hope that helps 👍

@Tokenyet
Copy link

Tokenyet commented Mar 1, 2022

I might make an issue, but It's maked as fixed, so I can't sure It's my problem or not. I have a test that used equatable but failed on nested iterable.

Logs:

   Which: at location [0] is ConsoleLine:<ConsoleLine((ConsoleText(null, null, false, 2022-03-01 16:08:51,840 main WARN Advanced terminal features are not available in this environment)))> instead of ConsoleLine:<ConsoleLine([ConsoleText(null, null, false, 2022-03-01 16:08:51,840 main WARN Advanced terminal features are not available in this environment)])>

The related classes:

class ConsoleLine extends Equatable {
  final Iterable<ConsoleText> texts;
  const ConsoleLine({
    required this.texts,
  });

  ConsoleLine copyWith({
    Iterable<ConsoleText>? texts,
  }) {
    return ConsoleLine(
      texts: texts ?? this.texts,
    );
  }

  @override
  List<Object> get props => [texts];
}

class ConsoleText extends Equatable {
  final Color? foreground;
  final Color? background;
  final bool bold;
  final String text;
  const ConsoleText({
    this.foreground,
    this.background,
    this.bold = false,
    required this.text,
  });

  @override
  List<Object?> get props => [foreground, background, bold, text];

  ConsoleText copyWith({
    Color? foreground,
    Color? background,
    bool? bold,
    String? text,
  }) {
    return ConsoleText(
      foreground: foreground ?? this.foreground,
      background: background ?? this.background,
      bold: bold ?? this.bold,
      text: text ?? this.text,
    );
  }
}

The generator method

  Iterable<ConsoleLine> parse(String text) sync* {
    final lines = const LineSplitter().convert(text);
    if (lines.isEmpty) return;
    for (final line in lines) {
      final parseResults = decodeAnsiColorEscapeCodes(line, _ansiUp);
      yield ConsoleLine(
        texts: parseResults.map(
          (e) => ConsoleText(
            bold: e.bold,
            text: e.text,
            foreground: _colorFromAnsi(e.fgColor),
            background: _colorFromAnsi(e.bgColor),
          ),
        ),
      );
    }
  }

If I change the generator method to the following, the test passed...

  Iterable<ConsoleLine> parse(String text) sync* {
    final lines = const LineSplitter().convert(text);
    if (lines.isEmpty) return;
    for (final line in lines) {
      final parseResults = decodeAnsiColorEscapeCodes(line, _ansiUp);
      yield ConsoleLine(
        texts: parseResults.map(
          (e) => ConsoleText(
            bold: e.bold,
            text: e.text,
            foreground: _colorFromAnsi(e.fgColor),
            background: _colorFromAnsi(e.bgColor),
          ),
        ).toList(),
      );
    }
  }

Or make the List<Object> get props => [texts]; to List<Object> get props => [...texts];, also pass the test.

Any help would be appreciated!

Edit:
Not sure what's the best way to handle this case in tests, but I change the form from

        expect(
          consoleRepository.parse(rawA),
          [
            ConsoleLine(
              texts: [
                const ConsoleText(
                    text:
                        '2022-03-01 16:08:51,840 main WARN Advanced terminal features are not available in this environment'),
              ],
            ),
          ],
        );

to

        expect(
          consoleRepository.parse(rawA),
          [
            ConsoleLine(
              texts: Iterable.castFrom([
                const ConsoleText(
                    text:
                        '2022-03-01 16:08:51,840 main WARN Advanced terminal features are not available in this environment'),
              ]),
            ),
          ],
        );

For whom stuck on nested iterable, I think here is the solution, might not be the best.

@felangel
Copy link
Owner Author

felangel commented Mar 1, 2022

@Tokenyet Equatable should support nested iterable props. I added more tests for this specific case in #135. Are you able to provide a unit test that fails to illustrate the problem you're facing? Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
equatable
  
Done
Development

Successfully merging this pull request may close these issues.

Equatable doesn't work with nested custom Iterable objects
3 participants