-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
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 |
2ad4eb6
to
77b9daf
Compare
Resolves crystal-lang#2155 Signed-off-by: Mike Fiedler <miketheman@gmail.com>
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So we don't need a reduce?(memo)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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