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

Use local variables in _form.html.erb generated by scaffold. #13434

Merged
merged 1 commit into from Jan 3, 2015

Conversation

@tanraya
Copy link
Contributor

@tanraya tanraya commented Dec 21, 2013

It seems a good idea to use local variables in generated partials instead of using instance variables.

Before

<%= render 'form' %>

After

<%= render 'form', product: @product %>
@@ -1,3 +1,7 @@
* Use local variables in _form.html.erb generated by scaffold generator.

This comment has been minimized.

@carlosantoniodasilva

carlosantoniodasilva Dec 21, 2013
Member

`_form.html.erb` partial generated by scaffold.
# Views local variables
assert_file "app/views/product_lines/_form.html.erb" do |test|
assert_no_match(/@product_line/, test)
end

This comment has been minimized.

@carlosantoniodasilva

carlosantoniodasilva Dec 21, 2013
Member

You can probably remove the _form file from line 66 since we're testing against the same file here again.

This comment has been minimized.

@carlosantoniodasilva

carlosantoniodasilva Dec 21, 2013
Member

Actually nevermind, leave it there.

@carlosantoniodasilva
Copy link
Member

@carlosantoniodasilva carlosantoniodasilva commented Dec 21, 2013

👍

@dmathieu
Copy link
Contributor

@dmathieu dmathieu commented Dec 22, 2013

@tanraya can you squash your commits please? Also, they don't apply cleanly into master. You'll need to rebase.

@tanraya
Copy link
Contributor Author

@tanraya tanraya commented Dec 22, 2013

@dmathieu I will do.

@tanraya
Copy link
Contributor Author

@tanraya tanraya commented Dec 28, 2013

@dmathieu done

@@ -68,6 +68,17 @@ def test_scaffold_on_invoke
end
assert_no_file "app/views/layouts/product_lines.html.erb"

# Views local variables
assert_file "app/views/product_lines/_form.html.erb" do |test|
assert_no_match(/@product_line/, test)

This comment has been minimized.

@senny

senny Jan 6, 2014
Member

you should verify what should be there, not what shouldn't be there.

This comment has been minimized.

@carlosantoniodasilva

carlosantoniodasilva Jul 28, 2014
Member

👍 for @senny's comment.

@jamo
Copy link
Contributor

@jamo jamo commented Feb 11, 2014

I added the suggested test case, see jamo@a0b548d

(all tests passes)

@arunagw
Copy link
Member

@arunagw arunagw commented Jul 4, 2014

@tanraya did you get some time to update this PR?

thanks

@tanraya
Copy link
Contributor Author

@tanraya tanraya commented Jul 25, 2014

@arunagw Yes. Updated now.

@arunagw
Copy link
Member

@arunagw arunagw commented Jul 25, 2014

@tanraya seems test broke. Can you see those as well ?

@carlosantoniodasilva
carlosantoniodasilva reviewed Jul 25, 2014
View changes
railties/lib/rails/generators/erb/scaffold/templates/_form.html.erb Outdated

<ul>
<%% @<%= singular_table_name %>.errors.full_messages.each do |message| %>
<li><%%= message %></li>
<%% end %>

This comment has been minimized.

@carlosantoniodasilva

carlosantoniodasilva Jul 25, 2014
Member

It seems this should not be removed.

It seems a good idea to use local variables in generated partials instead of using instance variables.

Before

    <%= render 'form' %>

After

    <%= render 'form', product: @Product %>
@tanraya
Copy link
Contributor Author

@tanraya tanraya commented Jul 26, 2014

@carlosantoniodasilva @arunagw Now its done. All tests passed.

@rafaelfranca rafaelfranca modified the milestones: 4.2.0, 5.0.0 Aug 18, 2014
carlosantoniodasilva added a commit that referenced this pull request Jan 3, 2015
Use local variables in _form.html.erb generated by scaffold.

Conflicts:
	railties/CHANGELOG.md
carlosantoniodasilva added a commit that referenced this pull request Jan 3, 2015
Related to #13434.
@carlosantoniodasilva carlosantoniodasilva merged commit 6bd8126 into rails:master Jan 3, 2015
1 check passed
1 check passed
continuous-integration/travis-ci The Travis CI build passed
Details
@rafaelfranca rafaelfranca modified the milestones: 5.0.0 [temp], 5.0.0 Dec 30, 2015
prathamesh-sonpatki added a commit to prathamesh-sonpatki/rails that referenced this pull request Jun 11, 2016
christiannelson added a commit to carbonfive/raygun-rails that referenced this pull request Jul 1, 2016
See rails/rails#13434 for reference.
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.