This project is archived and is in readonly mode.
fields_for nested fubars index
Reported by ara.t.howard | April 2nd, 2009 @ 10:11 PM | in 2.3.4
using fields_for with a nested attribute numbers the resource index incorrectly. although this does not cause an exception it does make scripting js to add additional form elements harder and more error prone than it needs to be since the form elements generated do reflect the size of the actual association.
Comments and changes to this ticket
-
José Valim April 2nd, 2009 @ 10:31 PM
+1
I've tested and this works well.
I just recommend you to put some parenthesis in your patch, to agree with Rails code:
def nested_child_index(association_name)
-
-
ara.t.howard April 2nd, 2009 @ 10:48 PM
- Tag changed from “fields_for, nested” to “fields_for, nested, patch”
-
-
-
Eloy Duran April 3rd, 2009 @ 08:08 PM
- State changed from “new” to “verified”
The change is good, however I have a few questions about the patch and took the liberty to rewrite it with these notes.
- There was no test for the case where the association would be passed in as a string and a symbol. However, the code seemed to indicate that you were, rightfully so, thinking of this case.
- Why did you choose to use "ensure"? There was at least no test which indicated why afaik.
- The test name did not really describe what it was gonna test.
Thoughts?
-
ara.t.howard April 3rd, 2009 @ 08:22 PM
There was no test for the case where the association would be passed in as a string and a symbol. However, the code seemed to indicate that you were, rightfully so, thinking of this case.
well, it isn't the case that the association should be a symbol or a string - it could be anything that responds to #to_s, a custom class, a stringio, etc. that's why there is no test for symbol explicitly but simply ducktype to string no matter what's passed in. for instance ['posts'] would work and so would [:posts]. this is useful in a *args case. however, testing all possible stringlike intputs seems unreasonable.
Why did you choose to use "ensure"? There was at least no test which indicated why afaik.
it's there because ruby lacks a prefix increment function (++i). the previous code uses the obfusication pattern of initializing with -1 and using +=, but that really doesn't read 'starts at zero and increments by 1'. ensure is a good way to implement prefix increment in ruby since it increments on the way out of the method, after the return value has been determined.
The test name did not really describe what it was gonna test
guilty as charged. ;-) not for lack of trying though!
thanks for the work on this - it really makes adding elements via js vastly simpler.
-
ara.t.howard April 3rd, 2009 @ 08:23 PM
oh crap. should have read 'Formatting help'. sorry bout that.
-
Eloy Duran April 3rd, 2009 @ 08:58 PM
well, it isn't the case that the association should be a symbol or a string - it could be anything that responds to #to_s, a custom class, a stringio, etc. that's why there is no test for symbol explicitly but simply ducktype to string no matter what's passed in. for instance ['posts'] would work and so would [:posts]. this is useful in a *args case. however, testing all possible stringlike intputs seems unreasonable.
True, I changed it back to #to_s again. But adding at least one different object in the test is at place.
it's there because ruby lacks a prefix increment function (++i). the previous code uses the obfusication pattern of initializing with -1 and using +=, but that really doesn't read 'starts at zero and increments by 1'. ensure is a good way to implement prefix increment in ruby since it increments on the way out of the method, after the return value has been determined.
Fair enough, I changed this back. Thanks for the trick ;-)
oh crap. should have read 'Formatting help'. sorry bout that.
Yeah the lack of a preview on LH doesn't help either…
guilty as charged. ;-) not for lack of trying though! thanks for the work on this - it really makes adding elements via js vastly simpler.
Yes definitely, thanks again.
-
Eloy Duran April 5th, 2009 @ 04:18 PM
- Assigned user changed from “Eloy Duran” to “Michael Koziarski”
-
-
Jarl Friis May 11th, 2009 @ 01:15 PM
May I suggest a slightly different patch for this problem. Take a look at the patch.
It uses the local string 'name' as key. It is more informative, but it may be slower, since it is not a symbol.
-
ara.t.howard May 11th, 2009 @ 04:24 PM
checkout my initial patch, it did exactly this. the concept simply got dropped when others hacked on it.
-
Jarl Friis May 12th, 2009 @ 11:12 AM
@ara.t.howard
Your initial patch uses "association_name" as argument to nested_child_index, whereas I suggest (in my patch) to use the local variable called "name" as argument to nested_child _index.
I fear that if not using a fully qualified parameter name (such as "name"), then using a unqualified value like association_name will eventually turn up again further down the parameter structure. If you are interested I will try to make a test that fails that illustrates the problem.
-
Eloy Duran May 12th, 2009 @ 11:16 AM
@Jarl You're right, it's a good improvement. Please do add the failing test you are talking about.
-
Jarl Friis May 13th, 2009 @ 02:17 PM
As per request by Eloy Duran.
I have changed the test (initially made by ara.t.howard) to a stricter test testing for the case where sub-associations have same names.
Now the test fails with the initial patch, but passes with my patch.
-
Michael Koziarski June 9th, 2009 @ 09:16 AM
- Milestone changed from “2.x” to “2.3.4”
Is this good to go eloy?
-
Repository July 2nd, 2009 @ 08:46 PM
- State changed from “verified” to “resolved”
(from [1c855ad4e7c14fbf08edc1f1eb9b3c6fe186b701]) My suggestion to fix ticket 2401 [#2401 state:resolved]
Signed-off-by: Yehuda Katz + Carl Lerche ykatz+clerche@engineyard.com
http://github.com/rails/rails/commit/1c855ad4e7c14fbf08edc1f1eb9b3c... -
Repository July 2nd, 2009 @ 08:46 PM
(from [e61afed6f8d2ef6a580ba00a5beaaaf0bf2ddb9a]) My suggestion to fix ticket 2401 [#2401 state:resolved]
Signed-off-by: Yehuda Katz + Carl Lerche ykatz+clerche@engineyard.com
http://github.com/rails/rails/commit/e61afed6f8d2ef6a580ba00a5beaaa...
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="https://github.com/rails/rails/issues">https://github.com/rails/rails/issues</a>
People watching this ticket
ara.t.howard
Claudio Poli
Eloy Duran
José Valim
Michael Koziarski
Attachments
Tags
Referenced by
-
2401
fields_for nested fubars index
(from [1c855ad4e7c14fbf08edc1f1eb9b3c6fe186b701])
My sugg...
-
2401
fields_for nested fubars index
(from [e61afed6f8d2ef6a580ba00a5beaaaf0bf2ddb9a])
My sugg...