Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added PartialIteration object used when rendering collections #7698

Closed
wants to merge 3 commits into from

Conversation

@joeljunstrom
Copy link
Contributor

@joeljunstrom joeljunstrom commented Sep 19, 2012

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.

@frodsan
Copy link
Contributor

@frodsan frodsan commented Oct 26, 2012

@frodsan
frodsan reviewed Oct 26, 2012
View changes
actionpack/test/controller/render_test.rb Outdated
@@ -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') ]

This comment has been minimized.

@frodsan

frodsan Oct 26, 2012
Contributor

Please, change this to the 1.9 hash syntax. Thanks.

@frodsan
frodsan reviewed Oct 26, 2012
View changes
actionpack/test/controller/render_test.rb Outdated
end

def partial_collection_with_as_and_iteration
render :partial => "customer_iteration_with_as", :collection => [ Customer.new("david"), Customer.new("mary"), Customer.new('christine') ], :as => :client

This comment has been minimized.

@frodsan

frodsan Oct 26, 2012
Contributor

ditto.

@frodsan
frodsan reviewed Oct 26, 2012
View changes
actionpack/lib/action_view/partial_iteration.rb Outdated
@@ -0,0 +1,19 @@
module ActionView
class PartialIteration

This comment has been minimized.

@frodsan

frodsan Oct 26, 2012
Contributor

Can you add documentation about this?

If you think this is not part of the public API, you can add a # :nodoc: directive.

Thanks.

This comment has been minimized.

@joeljunstrom

joeljunstrom Oct 26, 2012
Author Contributor

Yea I was unsure wether this should be documented or not. The methods are mentioned in the docs for render partial and I had a hard time finding examples of an similar utility value object. I'll go with nodoc for now.

@joeljunstrom
Copy link
Contributor Author

@joeljunstrom joeljunstrom commented Oct 26, 2012

Comments applied and rebased

@frodsan
Copy link
Contributor

@frodsan frodsan commented Oct 26, 2012

Please squash your commits.

@lucasuyezu
Copy link
Contributor

@lucasuyezu lucasuyezu commented Nov 29, 2012

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

@SKoschnicke
Copy link

@SKoschnicke SKoschnicke commented Nov 29, 2012

+1. This would be a big improvement.

@joeljunstrom
Copy link
Contributor Author

@joeljunstrom joeljunstrom commented Nov 29, 2012

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

@lucasuyezu
Copy link
Contributor

@lucasuyezu lucasuyezu commented Nov 29, 2012

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

@joeljunstrom
Copy link
Contributor Author

@joeljunstrom joeljunstrom commented Nov 29, 2012

Yea I just could'nt come up with what to put the method on =) Sounds good!

@lucasuyezu
Copy link
Contributor

@lucasuyezu lucasuyezu commented Nov 29, 2012

@joeljunstrom Done! Can I has repo acces?

@joeljunstrom
Copy link
Contributor Author

@joeljunstrom joeljunstrom commented Nov 30, 2012

@lucasuyezu sorry, I was sleeping =) You now have access!

@rafaelfranca
Copy link
Member

@rafaelfranca rafaelfranca commented Nov 30, 2012

@joeljunstrom @lucasuyezu guys, just ping me when done.

Thanks.

@joeljunstrom
Copy link
Contributor Author

@joeljunstrom joeljunstrom commented Nov 30, 2012

Will do

@lucasuyezu
Copy link
Contributor

@lucasuyezu lucasuyezu commented Dec 3, 2012

@rafaelfranca Would you like to take a look now? I believe it is ok.

@lucasuyezu
Copy link
Contributor

@lucasuyezu lucasuyezu commented Dec 5, 2012

@joeljunstrom @rafaelfranca hello? Is anyone there? 😟

@joeljunstrom
Copy link
Contributor Author

@joeljunstrom joeljunstrom commented Dec 19, 2012

@rafaelfranca & @lucasuyezu Sorry about the delay, I've rebased and squashed including lucasuyezu's additions (deprecating the use of the template_name_counter local. I'm a bit hesitant regarding the extra complexity surrounding the deprecation warning but maybe it does not matter much.

@carlosantoniodasilva
carlosantoniodasilva reviewed Dec 19, 2012
View changes
actionpack/lib/action_view/template.rb Outdated
@locals.map do |key|
if key =~ /(.*)_counter$/
<<-local_method
redefine_method(:#{key}=) { |new_value| @#{key}= new_value }

This comment has been minimized.

@carlosantoniodasilva

carlosantoniodasilva Dec 19, 2012
Member

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.

This comment has been minimized.

@carlosantoniodasilva

carlosantoniodasilva Dec 19, 2012
Member

Btw, you probably don't need to set the variable, you can delegate to #{key}_iteration.index, right?

This comment has been minimized.

@lucasuyezu

lucasuyezu Dec 19, 2012
Contributor

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?

This comment has been minimized.

@carlosantoniodasilva

carlosantoniodasilva Dec 19, 2012
Member

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.

This comment has been minimized.

@joeljunstrom

joeljunstrom Dec 19, 2012
Author Contributor

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.

This comment has been minimized.

@@ -0,0 +1,31 @@
require 'abstract_unit'
require 'action_view/partial_iteration'
class PartialIterationTest < ActiveSupport::TestCase

This comment has been minimized.

@carlosantoniodasilva

carlosantoniodasilva Dec 19, 2012
Member

Add space before this line, and remove space after class and before the last end (the class body).

def last?
index == size - 1
end

This comment has been minimized.

@carlosantoniodasilva

carlosantoniodasilva Dec 19, 2012
Member

No need for empty line.

@carlosantoniodasilva
Copy link
Member

@carlosantoniodasilva carlosantoniodasilva commented Dec 19, 2012

Nice @joeljunstrom, I'd like to get this in for Rails 4 👍

@josevalim
josevalim reviewed Mar 7, 2013
View changes
actionpack/lib/action_view/template.rb Outdated
@@ -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}

This comment has been minimized.

@josevalim

josevalim Mar 7, 2013
Contributor

I don't follow the need for this. Could someone please enlighten me?

This comment has been minimized.

@lucasuyezu

lucasuyezu Mar 7, 2013
Contributor

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.

@mattgoldman
Copy link

@mattgoldman mattgoldman commented Aug 1, 2013

any updates on this? is it pretty much good to go?

@joeljunstrom
Copy link
Contributor Author

@joeljunstrom joeljunstrom commented Aug 2, 2013

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 ActionView::Template without a major refactor of how locals are assigned to templates.

When looking at this again I have a hard time feeling the addition is worth hacking ActionView::Template.

So unless someone have a great idea of how to achieve this maybe we should close this for now 😕

@lucasuyezu
Copy link
Contributor

@lucasuyezu lucasuyezu commented Aug 2, 2013

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.

joeljunstrom and others added 3 commits Sep 16, 2012
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.
@lucasuyezu
Copy link
Contributor

@lucasuyezu lucasuyezu commented Aug 3, 2013

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

@joeljunstrom
Copy link
Contributor Author

@joeljunstrom joeljunstrom commented Aug 3, 2013

I guess we should squash it and apply carlosantoniodasilva's formatting comments if I forgot to do it =)

@SKoschnicke
Copy link

@SKoschnicke SKoschnicke commented Jul 16, 2014

What prevents this from being merged? Can I help?

rafaelfranca added a commit that referenced this pull request Jul 16, 2014
Squash and merge #7698 doing some improvements to the original
implementation.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants
You can’t perform that action at this time.