Rails Ruby on Rails → 3.1

Comments and changes to this ticket

• 

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

  • Importance changed from “” to “Low”

  What are you trying to fix with this patch?

• 

  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 September 11th, 2010 @ 07:26 AM

  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.

• 

  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.

• You flagged this item as spam.
  

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

• 

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

• You flagged this item as spam.
  

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

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

• 

  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 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 September 24th, 2010 @ 11:25 AM

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

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

  • Tag cleared.

  Automatic cleanup of spam.

• 

  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 February 9th, 2011 @ 12:31 AM

  • State changed from “open” to “stale”

• You flagged this item as spam.
  

  bingbing March 26th, 2011 @ 01:42 AM

  omega watch