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

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

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

wants to merge 1 commit into from

Conversation

niallsmart
Copy link
Contributor

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
Copy link
Collaborator

j15e commented Aug 18, 2013

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
Copy link
Contributor Author

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
Copy link
Collaborator

j15e commented Aug 18, 2013

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants