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 distinctBy #396

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add distinctBy #396

wants to merge 1 commit into from

Conversation

yongjhih
Copy link

@yongjhih yongjhih commented Jan 25, 2020

Stream.fromIterable(['123', 'abc']).distinctBy((value) => value.length),
        emitsInOrder(<dynamic>[
          '123',
          emitsDone
        ]));

@codecov-io
Copy link

Codecov Report

Merging #396 into master will decrease coverage by <.01%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master     #396      +/-   ##
==========================================
- Coverage   93.34%   93.34%   -0.01%     
==========================================
  Files          59       61       +2     
  Lines        2013     2043      +30     
==========================================
+ Hits         1879     1907      +28     
- Misses        134      136       +2

@frankpepermans
Copy link
Member

Heya,

Thanks for submitting a PR!

However, I think it's best if we try to keep the amount of operators to a minimum,
distinctBy(method) is only a shorthand to distinct((a, b) => method(a) == method(b)).

You could do similar things for all other operators too, to suit different special use cases, but in the end, the sheer amount of operators would be too overwhelming even to experienced users.

Plus, you could simply enable this extension in your own project easily.

@yongjhih
Copy link
Author

yongjhih commented Feb 13, 2020

In RxJava and RxJs, the distinct operator accepts a selector instead of an equation, that's inconsistent to this. IMO, we should somehow provide an equivalent method.

According to you mention that keep the amount of operators to a minimum. I think the mapTo(T) is a redundant operator by the way, because it's just a shorthand to map((it) => T).

And yes, that's why I created a simple package for that: https://github.com/yongjhih/flutter_ext/blob/master/streamx/lib/streamx.dart
https://github.com/yongjhih/flutter_ext/blob/master/rxdartx/lib/rxdartx.dart

@frankpepermans
Copy link
Member

Remember, rxdart is built on top of Dart Streams.

In Dart Streams, there's distinct. But with different behavior to Rx in general, where we have distinct and distinctUntilChanged.

So, since distinct is taken, we chose to add distinctUnique and gave it the same signature, i.e. an (a, b) => bool.

The above is confusing to begin with, if we then add alternatives using selectors, we'd get:
distinct, distinctBy, distinctUnique, distinctUniqueBy.

...which is my main concern here.

Maybe selectors in general need a separate issue, cos we'd want to be consistent throughout the whole library then.

Just a random thought, but we could make selector alternatives available in a separate lib, ie rxdart/selectors.dart which is an optional import.

That way the common operators would follow Dart standards, where the new import would follow classic Rx standards.

@hoc081098
Copy link
Collaborator

hoc081098 commented Feb 8, 2021

For anyone interested in this extension (distinctBy), consider my package https://github.com/hoc081098/rxdart_ext 😃

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

Successfully merging this pull request may close these issues.

None yet

4 participants