This project is archived and is in readonly mode.

#5599 ✓stale
Thiago Pradi

Use Array.wrap instead of Array()

Reported by Thiago Pradi | September 9th, 2010 @ 11:07 PM | in 3.1

Hi guys,

This patch removes all calls to Array() from actionpack, and replaces it with Array.wrap().

Comments and changes to this ticket

  • Santiago Pastorino

    Santiago Pastorino September 11th, 2010 @ 05:35 AM

    • Importance changed from “” to “Low”

    What are you trying to fix with this patch?

  • Jeff Kreeftmeijer

    Jeff Kreeftmeijer September 11th, 2010 @ 06:46 AM

    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? :)

  • Santiago Pastorino

    Santiago Pastorino September 11th, 2010 @ 07:26 AM

    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.

  • José Valim

    José Valim September 11th, 2010 @ 08:06 AM

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

  • Thiago Pradi

    Thiago Pradi September 11th, 2010 @ 02:13 PM

    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.

  • Andrew White

    Andrew White September 12th, 2010 @ 12:47 AM

    +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.

  • Aaron Patterson

    Aaron Patterson September 12th, 2010 @ 02:53 AM

    @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.

  • José Valim

    José Valim September 12th, 2010 @ 08:28 AM

    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().

  • Aaron Patterson

    Aaron Patterson September 12th, 2010 @ 10:33 PM

    @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.

  • José Valim

    José Valim September 13th, 2010 @ 08:11 AM

    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

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

  • Aaron Patterson

    Aaron Patterson September 13th, 2010 @ 04:59 PM

    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.

  • José Valim

    José Valim September 13th, 2010 @ 08:50 PM

    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?

  • Thiago Pradi

    Thiago Pradi September 13th, 2010 @ 10:44 PM

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

  • David Trasbo

    David Trasbo September 23rd, 2010 @ 10:22 AM

    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.

  • Jeremy Kemper

    Jeremy Kemper September 23rd, 2010 @ 05:44 PM

    • Milestone set to 3.1
    • State changed from “new” to “incomplete”

    Needs tests + rebase against latest master.

  • José Valim

    José Valim September 24th, 2010 @ 11:25 AM

    • Assigned user changed from “José Valim” to “Jeremy Kemper”
  • Jeff Kreeftmeijer

    Jeff Kreeftmeijer November 8th, 2010 @ 08:54 AM

    • Tag cleared.

    Automatic cleanup of spam.

  • Santiago Pastorino

    Santiago Pastorino February 9th, 2011 @ 12:31 AM

    • State changed from “incomplete” to “open”

    This issue has been automatically marked as stale because it has not been commented on for at least three months.

    The resources of the Rails core team are limited, and so we are asking for your help. If you can still reproduce this error on the 3-0-stable branch or on master, please reply with all of the information you have about it and add "[state:open]" to your comment. This will reopen the ticket for review. Likewise, if you feel that this is a very important feature for Rails to include, please reply with your explanation so we can consider it.

    Thank you for all your contributions, and we hope you will understand this step to focus our efforts where they are most helpful.

  • Santiago Pastorino

    Santiago Pastorino February 9th, 2011 @ 12:31 AM

    • State changed from “open” to “stale”
  • bingbing

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile »

<h2 style="font-size: 14px">Tickets have moved to Github</h2>

The new ticket tracker is available at <a href=""></a>