Use Array.wrap instead of Array()

While it doesn't fix any particular issue in this case, it seems like a good practice to use Array.wrap() instead of Array. Right? :)

http://ruby-doc.org/core/classes/Kernel.html#M005967
http://github.com/rails/rails/blob/master/activesupport/lib/active_...

Here you can find the differences. Kernel#Array can be used if it's appropriate, but what i'm asking for is ... if the current code using Kernel#Array is wrong, you should provide a test to demonstrate that.

Using Array.wrap is indeed a good practice. Some time ago JK mentioned we should always use it as pattern in Rails, so +1.

My idea is to use it as a pattern, preventing future problems. I could provide some tests to show wrong behaviors that this patch will fix.

+1 from me too - Action Pack is the only place that Array() is used instead of Array.wrap. I'd like to see some failing tests though.

@Thiago, would you mind providing those tests? Array() is faster than Array.wrap(). If we're going to slow something down, it would be nice to have tests demonstrating why we need to do that. Not to mention, if there are no tests, your patch could get removed in the future.

The places changed in the patch is not going to affect performance. They are not hot spots. This change should be faced as conceptual change. Array() does not mean wrap, although everyone thinks so. One example is thAt it breakes strings with \n into several ones.

For this reason, rails has Array.wrap() which is the right thing to call. We can surely provide a test case, but adding a test that passes a string with \n to callbacks just to ensure it won't be broken apart is a little bit crazy (since I don't expect no one to do that in real life). The justification For this patch being applied is to encourage everyone, including rails source code, to Use wrap instead of Array().

@Jose If something doesn't happen In Real Life™, why are you adding code to support it? Conceptual or not, the fact is that if a patch is applied that doesn't have corresponding tests, that patch could get removed in the future. If no tests fail, what would stop people from removing the code?

The rule should not be "Always use Array.wrap()". The rule should be "use the right tool for the job". If you actually need Array.wrap(), then providing tests shouldn't be problem: c/d.

Confirm and deny. For example, Rails guidelines says you cannot use
and in the source code. There is no way we can test that and it is
simply a concept/idea we have that it makes the code more
understandable.

I treat Array.wrap the same. We are using a explicit function that we
know what it does instead of Array() which behaves strangely.

and and && are control structures, not methods. We can easily test that someone uses Array() vs Array.wrap(). Array() behavior is known and documented.

Personally, I don't care which you use. What I do care about is that there are tests to reflect the code implementation. You can test that someone used Array() vs Array.wrap(), so you should do it.

In this case, the fact that you need to write tests that would never happen In Real Life™ brings in to question whether or not Array.wrap() is actually needed in these cases. But like I said, I really don't care what you use as long as there are tests so that people don't revert the change.

Guidelines apply to all code, I used control structures as an example.
:) and I still don't think all cases should be tested (and probably not all are possible to), but if it is a requisite for you I respect
that. :)

so can the patch be updated with tests for the cases where strings can
be given?

Sure, I have been busy lately, but the patch with tests is coming :)

As for the question of whether or not this patch would need tests, I definitely don't think so.

Think about it: 99 % of the time, when #Array is used within Rails what is the behavior we want from it? We want it to return an array containing whatever object we pass to it unless it's already an array. We don't need all the extra behavior #Array provides us with, such as splitting strings based on newlines, calling to_ary and to_a, and all the other stuff that might hide in there.

Array.wrap does what we need and nothing more, thus this patch does not fix a bug, per se - it's just a refactoring. If the tests still pass after the refactoring we're good to go.

omega watch