rstacruz / sinatra-assetpack

Package your assets transparently in Sinatra.
http://ricostacruz.com/sinatra-assetpack/
MIT License
542 stars 97 forks source link

Provide template filename to Sinatra/Tile via settings.assets #134

Open niallsmart opened 10 years ago

niallsmart commented 10 years ago

Compilation errors for dynamic templates always show class_methods.rb as the filename. This is because the filename option passed to render is never used (presumably it used to work, but it no longer does).

However per the commits below, it is possible to supply the template filename via settings.templates. This is something of an abuse of the intent of settings.templates, but given the benefit of showing the correct error information I think it's worth it.

j15e commented 10 years ago

Why use a proc? Are you sure this proc is working as expected? I would love to add a test related to this modification to make sure it really does what we think.

The :filename was added to the render options so it could be use in templates (see #126)

niallsmart commented 10 years ago

The reason I used a Proc here was because I figured the template_cache was already going to be caching the result of rendering the template, so there was no need to cache the contents of the template too. This also works:

settings.templates[fn.to_sym] = [File.read(fn), fn]

But then the contents of the File are just sitting around in memory for no good reason.

Re a testcase – if I create a unit test that tries and render a Sass stylesheet with an intentional SyntaxError then it can catch the exception and check the filename is correct (i.e., Sass::SyntaxError.filename). Is that what you were thinking of?

j15e commented 10 years ago

Yes exactly, a test for the wrong description in the exception