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

Controller Generator #507

Closed
wants to merge 3 commits into from
Closed

Controller Generator #507

wants to merge 3 commits into from

Conversation

@jeregrine
Copy link
Member

jeregrine commented Dec 5, 2014

Simple generator for single controllers.

mix phoenix.gen.controller controller_name [actions ... optional]

generates

web/controllers/controller_name_controller.ex
web/views/controller_name_view.ex
web/templates/controller_name/
test/controllers/controller_name_test.ex

The generator will try to be smart, and output the correct resource to std out. Here are the cases I am going to try and cover:

  • If the user enters no actions generate it as a resource in the controller and print to stdout the resource route to copy/paste into router.ex
  • If the user enters only valid resource actions I will generate a resource, the actions, and stdout the resource with the "only: [actions, user, choice]" parameter set.
  • If the user enters non-standard-resource actions I'll create them in the controller, and output a number of get '/controller_name/action', ControllerNameController :action with instructions to edit the get to the proper verb and copy into the router.

Potential issues/Questions:

  • I may need them to pass in the application name.

@josevalim @chrismccord lemme know what ya'll think. I think this is maybe step one for scaffolding, once we get ecto we can improve it even more.

TODO

  • Accept nested paths. EX: MyApp.API.V1.User => api_v1_user views/api/v1/user_view.ex etc
@chrismccord
Copy link
Member

chrismccord commented Dec 5, 2014

I think this is good way to get started on our generators. We could use it as a base to grow from once our Resource protocol is in place. A few thoughts:

web/templates/controller_name/error.html.eex
web/templates/controller_name/index.html.eex
web/templates/controller_name/not_found.html.eex

We should probably just generate an empty template dir, with a generated View.

I may want to add the resource automagically to the bottom of the router.ex file. Still uncertain.

This gets tricky with pipelines. Perhaps better would be to IO.puts a router entry that can be copy/pasted with instructions?

I may want to have the user specific "api" or "browser" and do different generators.

Depends since we handle this stuff at the router pipeline generally

I think this is maybe step one for scaffolding, once we get ecto we can improve it even more.

👍

@jeregrine
Copy link
Member Author

jeregrine commented Dec 5, 2014

@chrismccord thanks for the feedback! I updated the original issue to match your suggestions.

@josevalim
Copy link
Member

josevalim commented Dec 5, 2014

We should probably just generate an empty template dir, with a generated View.

We can also do it like Rails. We pass the controller name and then the actions as argument, each action gets a template.

@chrismccord
Copy link
Member

chrismccord commented Dec 5, 2014

We can also do it like Rails. We pass the controller name and then the actions as argument, each action gets a template.

I never really got into the extended argument support for any of Rails' generators since I never could remember and it took longer to look up what they did than just do it. I'm not opposed to it, just personal experience. If we do generate "default" actions like create/update/destroy/index, having default templates might be nice, but if you don't need all actions, you might after to delete files manually (which the explicit args could prevent). I'll let others chime. I'm game either way.

@josevalim
Copy link
Member

josevalim commented Dec 5, 2014

Since this is not about a resource, we should not generate any action by
default, unless one is given. Alternatively, we can generate the index
action and nothing more, otherwise we can't even give a route for people to
copy and paste.

José Valimwww.plataformatec.com.br
http://www.plataformatec.com.br/Founder and Lead Developer

@jeregrine
Copy link
Member Author

jeregrine commented Dec 5, 2014

well I was thinking resource "/controller_name" and generating all of the routes a resource needs with empty implementations. Or maybe just responding with a string.

The thought being it would partially document the routes that a router resource expects. If someone wanted to break it up they could easily do so.

@jeregrine
Copy link
Member Author

jeregrine commented Dec 5, 2014

Sorry I was thinking of generating a full "resource" controller with all the actions that a controller needs with empty or "string" implementations.

Again with the goal of reducing tedious boilerplate and education for documentation.

@jeregrine
Copy link
Member Author

jeregrine commented Dec 5, 2014

Started work on this and converted it to a PR. Next step is to test it.

@jeregrine
Copy link
Member Author

jeregrine commented Dec 6, 2014

Alright this has been tested, I wanted to get the "first version" out there. Easy to change and experiment from here.

@napcs
Copy link
Contributor

napcs commented Dec 6, 2014

Thoughts:

The generator would assume its a "resource" and add empty implementations.

I don't think this should be a resource unless it's called gen.resource. It's just a controller, so a simple route would do.

I'd love to be able to specify specific views like Rails

phoenix.gen.controller index help about

or however that works.
And that would generate the appropriate simple routes.

Other than that I think this is cool!

@jeregrine
Copy link
Member Author

jeregrine commented Dec 6, 2014

Alright so I added an update about generating actions, and what route to output to STDOUT. Please read the bullet points below the code block and let me know what you think.

@napcs

This comment has been minimized.

Copy link
Contributor

napcs commented on lib/mix/tasks/phoenix.gen.controller.ex in e594180 Dec 6, 2014

"creates a new Phoenix controller"?

def edit(conn, _params) do
end

# POST /<%= controller %>/new

This comment has been minimized.

@josevalim

josevalim Dec 7, 2014 Member

This is a get, no?

Mix.shell.info """
Don't forget to add your new controller to your web/router.ex
resource "/#{controller}", #{application_name}.#{controller_name}Controller

This comment has been minimized.

@josevalim

josevalim Dec 7, 2014 Member

resources

This comment has been minimized.

@josevalim

josevalim Dec 7, 2014 Member

You probably want to remove the application_name. bit too since in new applications the generated scopes are already aliased to the application name.

"""
end

defp copy_from(source_dir, target_dir, controller_name, fun) do

This comment has been minimized.

@josevalim

josevalim Dec 7, 2014 Member

We should move this into a module so it can be reused by phoenix.new and this one. Maybe moving it to the existing Mix.Phoenix module will be fine.


@shortdoc "Creates Phoenix controller"

@moduledoc """

This comment has been minimized.

@josevalim

josevalim Dec 7, 2014 Member

We need better docs. It doesn't just generate a controller, it also generates a view and a bunch of actions in the controller.

@josevalim
Copy link
Member

josevalim commented Dec 7, 2014

I am with @napcs on this one. This would be called a resource in Rails-land. So we need to decide if we want to stick with Rails convention and, if not, what is the plan if we are going to depart from it.

@jeregrine
Copy link
Member Author

jeregrine commented Dec 7, 2014

Thanks for the code feedback guys.

@josevalim I have kind of a plan in my updated PR comment, specifically the bullet points. I think for now I am leaning towards breaking away and sticking to generating a controller/actions. Once we have the resources protocol fleshed out it makes more sense to revisit this.

@jeregrine
Copy link
Member Author

jeregrine commented Dec 7, 2014

I agree on the docs. Once we nail down what we want this to ultimately do I can update the docs. Thanks for the feedback you guys! <3

@josevalim
Copy link
Member

josevalim commented Dec 7, 2014

Thanks @jeregrine, I just read the plan. I don't like it because it is very confusing what is going to be generated. I would prefer to have two generators then (one for resource and another for controllers). :) That said, my vote is to keep this resource specific and call it the resource generator. No need for options nor extra arguments.

@jeregrine
Copy link
Member Author

jeregrine commented Feb 10, 2015

I am going to close this and direct everyone to https://github.com/nickgartmann/ashes

@jeregrine jeregrine closed this Feb 10, 2015
@chrismccord chrismccord deleted the controller_generator branch Mar 13, 2015
Gazler pushed a commit that referenced this pull request Sep 6, 2017
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

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