Added PartialIteration object used when rendering collections #7698
Conversation
|
/cc @josevalim @drogus @jeremy |
| @@ -630,6 +630,14 @@ def partial_collection_with_as | |||
| render :partial => "customer_with_var", :collection => [ Customer.new("david"), Customer.new("mary") ], :as => :customer | |||
| end | |||
|
|
|||
| def partial_collection_with_iteration | |||
| render :partial => "customer_iteration", :collection => [ Customer.new("david"), Customer.new("mary"), Customer.new('christine') ] | |||
There was a problem hiding this comment.
Please, change this to the 1.9 hash syntax. Thanks.
|
Comments applied and rebased |
|
Please squash your commits. |
|
+1 for this pull request. Since PartialIteration implements #index, I believe the best solution regarding variable_counter would be to print/log a deprecation warning and remove it in 4.1 or another release. I could try to implement this if you guys want. Also let me know if I can help with anything. |
|
+1. This would be a big improvement. |
|
@lucasuyezu In the current implementation the counter is just a local variable making it a bit awkward adding a deprecation warning when accessing it. If you have ideas on a implementation I can give you access to my repo. |
|
@joeljunstrom We can turn the counter into a method with the same name that prints/logs the deprecation warning and return the variable. I will clone your repo and implement it. |
|
Yea I just could'nt come up with what to put the method on =) Sounds good! |
|
@joeljunstrom Done! Can I has repo acces? |
|
@lucasuyezu sorry, I was sleeping =) You now have access! |
|
@joeljunstrom @lucasuyezu guys, just ping me when done. Thanks. |
|
Will do |
|
@rafaelfranca Would you like to take a look now? I believe it is ok. |
|
@joeljunstrom @rafaelfranca hello? Is anyone there? 😟 |
|
@rafaelfranca & @lucasuyezu Sorry about the delay, I've rebased and squashed including lucasuyezu's additions (deprecating the use of the |
| @locals.map do |key| | ||
| if key =~ /(.*)_counter$/ | ||
| <<-local_method | ||
| redefine_method(:#{key}=) { |new_value| @#{key}= new_value } |
There was a problem hiding this comment.
I don't think we should make a setter method with an instance var for this, it'd be better to define everything in the same locals_code loop somehow.
There was a problem hiding this comment.
Btw, you probably don't need to set the variable, you can delegate to #{key}_iteration.index, right?
There was a problem hiding this comment.
I could try to use method_missing in the view object. In this scenario I would check if the method called is template_counter, warn and then return the value, making the redefine_method getters and setters unnecessary. But I still would need to keep either template_counter or template_iteration as an instance variable, because method_missing won't have access to local variables.
Does this sound better?
There was a problem hiding this comment.
I think we should avoid method missing if possible, but I don't have an idea off the top of my head, I'll try to think about something.
There was a problem hiding this comment.
I'm toying around with removing the name_counter variable from the partial renderer / locals and just setting up a method delegating to the iterator index. But tbh at the moment the module eval context in the template class is doing my head in =) Also it feels really dirty adding code to the template class for something that is just used for partials with collections, but I do not see a different way to do it yet.
|
Nice @joeljunstrom, I'd like to get this in for Rails 4 👍 |
| @@ -281,6 +281,7 @@ def #{method_name}(local_assigns, output_buffer) | |||
| ensure | |||
| @virtual_path, @output_buffer = _old_virtual_path, _old_output_buffer | |||
| end | |||
| #{define_locals_code} | |||
There was a problem hiding this comment.
I don't follow the need for this. Could someone please enlighten me?
There was a problem hiding this comment.
We want to show a deprecation warning when a user calls variable_counter instead of the new variable_iteration.index.
Since counter variables are local variables, this code defines a getter method for counter variables so we can warn and then return its original value.
|
any updates on this? is it pretty much good to go? |
|
The problem is that getting this functionality with an deprecation warning for the old counter variable is as I see it pretty much impossible without adding (some what hard to follow) code to When looking at this again I have a hard time feeling the addition is worth hacking So unless someone have a great idea of how to achieve this maybe we should close this for now 😕 |
|
The code is pretty much good to go, apart from the deprecation warning. I can revert it so this can ship, and prepare the deprecation warning on another pull request, if everybody agrees. |
The iteration object is available as the local variable "template_name_iteration" when rendering partials with collections. It gives access to the +size+ of the collection beeing iterated over, the current +index+ and two convinicence methods +first?+ and +last?+ "template_name_counter" variable is kept but is deprecated.
|
@joeljunstrom I have removed the deprecation warning. I also rebased the code based on the current rails master and fixed a spec that broke since then. Please let me know if there is any other thing blocking this. |
|
I guess we should squash it and apply carlosantoniodasilva's formatting comments if I forgot to do it =) |
|
What prevents this from being merged? Can I help? |
Squash and merge #7698 doing some improvements to the original implementation.
Based on the ideas in #5634 but refactored into an object as per the comments.
The iteration object is available as the local variable
"template_name_iteration" when rendering partials with collections.
It gives access to the +size+ of the collection beeing iterated over,
the current +index+ and two convinicence methods +first?+ and +last?+
For now the "template_name_counter" variable is kept but is deprecated.