Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Changed scaffold_controller generator to use respond_with and respond_to #233

Closed
wants to merge 2 commits into from

4 participants

@Breccan

Current scaffold_controller generates old code with lots of cruft. Changed to use respond_to and respond_with instead of the huge format blocks.

@radar

I helped Breccan with this one.

We both (and other people I've talked to) think the people using scaffolding would really appreciate it if there was less code that was generated when a scaffold was run. Currently, the scaffold generator hasn't changed much at all since 2.3 and still contains the ugly respond_to blocks.

This pull request first removes the comments from the scaffold controller (people should know this already, and if not they can learn from the Routing and Getting Started guides). That's the simple part.

The second commit here removes the ugly respond_to blocks, replacing it with a controller-level respond_to call, which is exactly how the respond_to blocks act. Well, at least as far as our testing goes.

Two more things for further concern.

First is that the scaffold generator generates functional tests for the HTML component that's generated, but doesn't generate XML tests. Is there any particular reason for this?

Second: Is there any particular reason why these controllers respond to XML rather than JSON these days? I would think that JSON is the preferred API respond format. This could just be a case of extreme bikeshedding though.

@dougireton

+1 for the Scaffold generator using JSON and not XML

@josevalim
Owner

This discussion appears from time to time. The default scaffold is mainly a learning tool. respond_with does a lot implicitly and would be harder for a beginner to understand, that's we still using respond_to. XML or JSON does not really matter, the point here is to show developers that there are to main kinds of formats: navigational and api. Changing XML for JSON at this point, would also "deprecate" all tutorials and books, which is not worth considering the default scaffold is just a learning tool anyway.

If you want to change scaffold, you can create your own generator and customize your app to use, at least this is what I did with the responders gem (http://github.com/plataformatec/responders).

@josevalim josevalim closed this
@radar

Ah, good to see the Rails team is now concerned with the deprecation of books. This makes me feel safer.

Thanks for the reply & feedback José.

@josevalim
Owner

Just to make it explicit: I don't think we should hold back adding/changing a feature in Rails to avoid deprecating books/tutorials/blog posts. However, given the default scaffold is exactly a learning tool and is one of the things that is first covered, I don't think trading XML by JSON is a worth change in a 3.0 to 3.1 release. A counter-example is exactly the changes the scaffold views had from 2.3 to 3.0.

@Breccan

The problem for me at least is that it seems like a huge number of people are using scaffold generator as a day to day tool in their work. With a room of 20 rails devs at railscamp we had a decent quantity who expressed surprise when Ryan pointed out that you shouldn't be using the current generators as a day to day development tool.

Certainly my experience shows that people are using it in business quite a lot. I've seen quite a few cases where the explicit commenting of routes above the methods has become the documentation standard throughout the app.

It's my belief that it's being used pretty heavily as something other than a learning tool, but I can't really prove it.

Cheers.

@josevalim
Owner

Hey Breccan, thanks for the pull request. However, wrong usage does not justify the change. For example, I have seen my share of people using MVC wrongly and there is nothing we can do besides instructing people to do the proper thing. The tools to extend scaffold beyond a learning tool are available, we just need to instruct people to use them properly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Mar 22, 2011
  1. @Breccan
  2. @Breccan

    Updated scaffold_controller generator to use respond_to and

    Breccan authored
    respond_with (ht: radar)
This page is out of date. Refresh to see the latest.
View
62 railties/lib/rails/generators/rails/scaffold_controller/templates/controller.rb
@@ -1,85 +1,49 @@
<% module_namespacing do -%>
class <%= controller_class_name %>Controller < ApplicationController
- # GET <%= route_url %>
- # GET <%= route_url %>.xml
+ respond_to :html, :xml
+
def index
@<%= plural_table_name %> = <%= orm_class.all(class_name) %>
-
- respond_to do |format|
- format.html # index.html.erb
- format.xml { render :xml => @<%= plural_table_name %> }
- end
+ respond_with(@<%= plural_table_name %>)
end
- # GET <%= route_url %>/1
- # GET <%= route_url %>/1.xml
def show
@<%= singular_table_name %> = <%= orm_class.find(class_name, "params[:id]") %>
-
- respond_to do |format|
- format.html # show.html.erb
- format.xml { render :xml => @<%= singular_table_name %> }
- end
+ respond_with(@<%= singular_table_name %>)
end
- # GET <%= route_url %>/new
- # GET <%= route_url %>/new.xml
def new
@<%= singular_table_name %> = <%= orm_class.build(class_name) %>
-
- respond_to do |format|
- format.html # new.html.erb
- format.xml { render :xml => @<%= singular_table_name %> }
- end
+ respond_with(@<%= singular_table_name %>)
end
- # GET <%= route_url %>/1/edit
def edit
@<%= singular_table_name %> = <%= orm_class.find(class_name, "params[:id]") %>
+ respond_with(@<%= singular_table_name %>)
end
- # POST <%= route_url %>
- # POST <%= route_url %>.xml
def create
@<%= singular_table_name %> = <%= orm_class.build(class_name, "params[:#{singular_table_name}]") %>
- respond_to do |format|
- if @<%= orm_instance.save %>
- format.html { redirect_to(@<%= singular_table_name %>, :notice => '<%= human_name %> was successfully created.') }
- format.xml { render :xml => @<%= singular_table_name %>, :status => :created, :location => @<%= singular_table_name %> }
- else
- format.html { render :action => "new" }
- format.xml { render :xml => @<%= orm_instance.errors %>, :status => :unprocessable_entity }
- end
+ if @<%= orm_instance.save %>
+ flash[:notice] = "<%= human_name %> successfully created."
end
+ respond_with(@<%= singular_table_name %>)
end
- # PUT <%= route_url %>/1
- # PUT <%= route_url %>/1.xml
def update
@<%= singular_table_name %> = <%= orm_class.find(class_name, "params[:id]") %>
- respond_to do |format|
- if @<%= orm_instance.update_attributes("params[:#{singular_table_name}]") %>
- format.html { redirect_to(@<%= singular_table_name %>, :notice => '<%= human_name %> was successfully updated.') }
- format.xml { head :ok }
- else
- format.html { render :action => "edit" }
- format.xml { render :xml => @<%= orm_instance.errors %>, :status => :unprocessable_entity }
- end
+ if @<%= orm_instance.update_attributes("params[:#{singular_table_name}]") %>
+ flash[:notice] = "<%= human_name %> successfully created."
end
+ respond_with(@<%= singular_table_name %>)
end
- # DELETE <%= route_url %>/1
- # DELETE <%= route_url %>/1.xml
def destroy
@<%= singular_table_name %> = <%= orm_class.find(class_name, "params[:id]") %>
@<%= orm_instance.destroy %>
-
- respond_to do |format|
- format.html { redirect_to(<%= index_helper %>_url) }
- format.xml { head :ok }
- end
+ respond_with(@<%= singular_table_name %>)
end
end
<% end -%>
View
2  railties/test/generators/scaffold_controller_generator_test.rb
@@ -35,13 +35,11 @@ def test_controller_skeleton_is_created
assert_instance_method :create, content do |m|
assert_match /@user = User\.new\(params\[:user\]\)/, m
assert_match /@user\.save/, m
- assert_match /@user\.errors/, m
end
assert_instance_method :update, content do |m|
assert_match /@user = User\.find\(params\[:id\]\)/, m
assert_match /@user\.update_attributes\(params\[:user\]\)/, m
- assert_match /@user\.errors/, m
end
assert_instance_method :destroy, content do |m|
Something went wrong with that request. Please try again.