Adds Enumerable#reduce? for when input is empty #4828
Conversation
|
Found via CodeTriage #triage and tried to make the PR very similar to the implementation of |
miketheman
force-pushed
the
miketheman/empty-reduce
branch
from
2ad4eb6
to
77b9daf
Compare
Resolves crystal-lang#2155 Signed-off-by: Mike Fiedler <miketheman@gmail.com>
miketheman
force-pushed
the
miketheman/empty-reduce
branch
from
77b9daf
to
cb07bff
Compare
| end | ||
|
|
||
| # Similar to `reduce?`, however instead of returning `nil` when the input is empty, | ||
| # return the initial value of the accumulator. |
There was a problem hiding this comment.
This is incorrect, it's exactly the same method implementation as reduce above. Actually the original impl of reduce(memo) is incorrect and does not raise Enumerable::EmptyError at all.
There was a problem hiding this comment.
From the docs and tests for reduce(memo), it appears that returning the memo is desired rather than raise https://github.com/crystal-lang/crystal/pull/4828/files#diff-6519b36b7135a1958e191f91f98354d8L560
There was a problem hiding this comment.
So we don't need a reduce?(memo)?
There was a problem hiding this comment.
Maybe not? I wasn't certain of how a user might write their code, and how this might be used in that regard. I can remove the reduce?(memo) if that's the right thing.
There was a problem hiding this comment.
If there is an initial value for memo, there is no need to raise or return nil, it should always return that value if empty.
There was a problem hiding this comment.
Well the implementations are exactly the same so i'm pretty sure it's the right thing to do.
There was a problem hiding this comment.
reduce?(memo) should be removed since it's identical to reduce(memo).
|
two years ago ... why didn't merge? |
|
😢 |
|
@miketheman i can re-open if you want to continue this PR. The review comments are still outstanding. |
|
@RX14 I need this feature ... could you ... just merge it? |
Resolves #2155
Signed-off-by: Mike Fiedler miketheman@gmail.com