Move Array#product to Enumerable #3794
Conversation
|
Thoughts? Is checking for #size ok? |
| @@ -9,7 +9,7 @@ | |||
| ## Build the compiler in release mode | |||
| ## $ make crystal release=1 | |||
| ## Run all specs in verbose mode | |||
| ## $ make specs verbose=1 | |||
| ## $ make spec verbose=1 | |||
There was a problem hiding this comment.
Please separate this fix from this pull request!
This allows doing cartesian products on things that are not arrays. It should also reduce the confusion of Array having product methods from two different locations.
| # | ||
| # An empty array will be returned if either collection is empty. | ||
| def product(ary : Enumerable(U)) forall U | ||
| result = if ary.responds_to?(:size) |
There was a problem hiding this comment.
Please use this syntax instead:
if ary.responds_to?(:size)
result = Array({T, U}).new(size * ary.size)
else
result = Array({T, U}).new
endIt's much cleaner with the indenting.
There was a problem hiding this comment.
This is not quite correct. Enumerable also has a size method, that merely iterates through all of the elements to count them. So this will traverse the enumerable many times.
I think product is fine in Array, and shouldn't be available in Enumerable.
We'll eventually revisit all of the std design, in one go, instead of deciding the fate of each method case by case. I don't think I'll be able to merge this PR or take any other decision about this right now.
There was a problem hiding this comment.
Ouch, that would be a bad side effect without any obvious solution. Should I close or leave the pull request?
Or should I separate out the documentation additions as those may be nice even if it isn't moved to enumerator?
| # An empty array will be returned if either collection is empty. | ||
| def product(ary : Enumerable(U)) forall U | ||
| result = if ary.responds_to?(:size) | ||
| Array({T, U}).new(size * ary.size) |
There was a problem hiding this comment.
Here you assume both ary and this class respond to size, but only check for ary having size.
|
|
||
| product(ary) do |x, y| | ||
| result << {x, y} | ||
| end |
|
I think it would be extremely wise to rename this |
|
#codetriage |
|
Any update on this? WDYT @asterite ? |
|
Let's close this PR as the issue regarding |
Issue #3790