Add argument to Array#flatten to specify amount of levels to flatten. #3541
Conversation
|
Hi @yxhuvud, the PR introduce some subtle but important issues regarding the type of the results. Maybe a spec should be added to the existing flatten method. Before this PR the following holds: typeof([[1, 'a'], [[[[true], "hi"]]]].flatten).should eq(Array(Int32 | Char | Bool | String))After your PR it does not. If I add that assertion to array_spec.cr:1678 : At the end of the day the following snippet is the cause of why the above assertion does not hold in this PR: def foo(n)
if n == 1
3
else
[4]
end
end
typeof(foo(1)) # => Int32 | Array(Int32)It is important that the returning types are useful. Most of the time this means no union types, otherwise the user will need to cast/case the return value before using it at all. It is impossible for crystal to determine the compile time type based on the values (number of steps to flatten). If the case where only one step is need I would create a different operation. Maybe based on inject/concat. |
|
@bcardiff Ok, yeah that makes a lot of sense. Hmm - for the single level case I assumed that flat_map &.itself would suffice, but that one seems to require a single type for each subarray (nonarray root objects work fine) |
|
I agree that Thanks! even if the PR didn't make it, it is still a contribution. |
| def flatten | ||
| FlattenHelper(typeof(FlattenHelper.element_type(self))).flatten(self) | ||
| def flatten(steps : Int = -2) | ||
| FlattenHelper(typeof(FlattenHelper.element_type(self, steps))).flatten(self, steps) |
There was a problem hiding this comment.
I am guessing might want to trap for steps <= -1 here [probably better performance]; otherwise, you'll probably want to do something like if steps <= -1 checks in the loops below in FlattenHelper [probably a bit slower]; otherwise FlattenHelper's loops could continue towards -infinity [well probably -max int].
There was a problem hiding this comment.
No, they can only continue until the arrays are flat. Note that the base case without arguments is -2.
But as bcardiff noted, the real issue is that the type signature is garbled.
Ruby 1.8.7 added an optional numerical argument to Array#flatten to specify the amount of array levels that should be present. In practice, I've found that I almost always use flatten with an argument of 1 - in some situations it may protect against people that input arrays where the whole array shouldn't be flattened but just the topmost level.
Regarding the implementation, I'm not all that happy with the recursion criteria. It is pretty trivial apart from that. It may be a good idea to add tests for the types of the array, but I am not certain how to write that so I left it out for now.