(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.)

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?

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

@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

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

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

@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

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

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 ?

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

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?

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

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

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.