Avoid Helpers::Render in mandatory includes

[docelic]

This patch moves "include Helpers::Render" from Amber::Controller::Base
into ApplicationController.

ApplicationController is part of user code and now it is possible to
avoid including Helpers::Render (and kilt dependencies) in case that
user application does not want to use the default render system.

[docelic]

(Just to be clear, neither @robacarp nor I know how the "in-progress" tag got attached. The PR is in any case finished and ready for review.)

[drujensen]

I like this. ❤️

[drujensen]

Should we add an ApplicationController fixture in between?

[docelic]

Good one, will do, thanks!

[docelic]

Actually @drujensen looking into spec/support/fixtures/controller_fixtures.cr I suggest things to remain as they are.

Because if we add ApplicationController fixture then other fixture classes in that file should inherit from it, which would make them have more functionality than the spec specifically wanted to test.

So I would for keeping specs as granular and specific as they are now.

[marksiemers]

The failures in Travis are real. I think the error controller may not inherit from ApplicationController and it calls render.

Perhaps the best fix is to have the ErrorController include the render helper directly. @drujensen @eliasjpr - WDYT?

[docelic]

@marksiemers uh yes, but if ErrorController needs it, then that means Amber's render helper will always be "required" somewhere and it will bring in kilt dependencies with it.

(In other words, if we can't remove use of render() from there, then it's not truly possible to completely avoid Amber's built-in rendering (which as a consequence requires kilt).)

Even though, by removing the include from AppController the user can essentially free his part of code the built-in "render" macro and all other things that get initialized, which was the most important to do anyway, so it still looks useful.

[marksiemers]

@docelic - Let me clarify, it isn't Amber's Amber::Controller::Error that needs it.

If you run amber g error it will create this:

class ErrorController < Amber::Controller::Error
  LAYOUT = "application.slang"

  def forbidden
    render("forbidden.slang")
  end

  def not_found
    render("not_found.slang")
  end

  def internal_server_error
    render("internal_server_error.slang")
  end
end

For a little background, what the build tests do is build an actual amber app using the cli, then it generates one of everything (model, controller, scaffold, auth, etc.). Then it runs the test suite of that generated app.

You can recreate locally with crystal spec -D run_build_tests

[marksiemers]

So, just the generator will need to be updated for amber g error

[faustinoaq]

So, just the generator will need to be updated for amber g error

Good catch @marksiemers 👍

@docelic Yeah just add include Amber::Controller::Helpers::Render before this line: https://github.com/docelic/amber/blob/547809cb2bf43391523334426f20b87b5989d2ad/src/amber/cli/templates/error/src/controllers/%7B%7Bname%7D%7D_controller.cr.ecr#L2

[faustinoaq]

ErrorController needs include Amber::Controller::Helpers::Render as well

[faustinoaq]

@docelic Thank you for fixing ErrorController, Looks like this spec need to be updated as well:

  1) Amber::CLI::MainCommand::Generate amber generate error in an `amber new` app with default options generates expected controller class
     Failure/Error: File.read("./src/controllers/error_controller.cr").should eq expected
       Expected: "class ErrorController < Amber::Controller::Error\n  LAYOUT = \"application.slang\"\n\n  def forbidden\n    render(\"forbidden.slang\")\n  end\n\n  def not_found\n    render(\"not_found.slang\")\n  end\n\n  def internal_server_error\n    render(\"internal_server_error.slang\")\n  end\nend\n"
            got: "require \"amber/controller/helpers/render\"\n\nclass ErrorController < Amber::Controller::Error\n  include Amber::Controller::Helpers::Render\n  LAYOUT = \"application.slang\"\n\n  def forbidden\n    render(\"forbidden.slang\")\n  end\n\n  def not_found\n    render(\"not_found.slang\")\n  end\n\n  def internal_server_error\n    render(\"internal_server_error.slang\")\n  end\nend\n"

Spec location: spec/amber/cli/commands/generate/error_template_spec.cr:19

[docelic]

I seem to be able to run tests correctly now. Will fix failing specs in other PRs now too.

[elorest]

I would very much prefer to keep render in Controller::Base for now. It's not in the way of Kilt as you can still call Kilt.render or overload your own render macro if necessary.

If we start removing something as default as render, why not redirect, params, before_filters, csrf, responders, the router or anything else that makes amber what it is?

[faustinoaq]

If we start removing something as default as render, why not redirect, params, before_filters, csrf, responders, the router or anything else that makes amber what it is?

As @docelic said:

in case that user application does not want to use the default render system.

Perhaps some render systems can conflict with Kilt ?

[elorest]

@faustinoaq I haven't seen any render systems that do conflict with kilt, since it's namespaced under Kilt and even if they did it doesn't really mean anything. Almost every part of Amber, Rails or any other framework will conflict with a library trying to do the exact same thing as a part of it.

[robacarp]

I think I side with @elorest here. If the default render macro doesn’t do what you want, why not just override it in your application controller?

[docelic]

Well you get render() macro from default method and possibly some other stuff that it initializes in the controller.
Ok, closing the PR then.

[elorest]

@docelic Thanks for your work on this. Personally I didn't really want this to happen, but I hate it when someone puts a lot of time into code and doesn't get to use it. I just want you to know that I really do appreciate it. This may be something we'll want to address in the future. My biggest issues were with it right now is that there are multiple things on the same level and usefulness in the base controller and just moving one of them out seemed a little premature. Thanks.

[docelic]

Sure, no worries. I instead sent additional patches to Liquid.cr and Kilt to make them work as one would expect (including ability to pass custom context), so there's no need yet to replace/leave out kilt in any scenario.