Enhancing Templates#render's abilities #531

Closed
wants to merge 12 commits into
from

Projects

None yet

6 participants

@blambeau

This pull request towards the following features (see #529 for original motivation):

  • ability to render a template from a path, as Tilt already allows, with the cache automatically enabled
  • ability to render a template without knowing the corresponding engine a priori. A logical consequence of the first point.

The pull request mostly extends the public API, in a backward compatible way, as follows:

def render(view, options={}, locals={}, &block)
    # ...
end

The render method becomes public. It also accepts a new signature allowing to omit the engine argument. The idea is to allow new calls such as:

render engine, path_to_a_file    # where path_to_a_file is a recognized path object
render path_to_a_file            # the engine is inferred using Tilt
render :view_name                # the engine is also inferred from the existing file (ambiguous if more than one)

The implementation mostly replaces compile_template by a new greedy algorithm called tilt_template. The latter recursively discovers the parameters needed by Tilt::Template.new. The stuff seems fully backward compatible and passes all the tests. compile_template is kept for backward compatibility.

@travisbot

This pull request passes (merged 26464bb into 8752085).

@travisbot

This pull request fails (merged 963c145 into 8752085).

@travisbot

This pull request passes (merged 38de911 into 8752085).

@travisbot

This pull request passes (merged e49e540 into 8752085).

@travisbot

This pull request passes (merged ec8a756 into 8752085).

blambeau added some commits Jun 14, 2012
@blambeau blambeau Factorize path-driven code in template_tests. e7cb8f9
@blambeau blambeau Factorize path extraction from a view argument. 53846ac
@blambeau blambeau Promote `render` in public API, with generalized signature.
A private method `_render(view, options, &block)` replaces the previous
render method. The engine to use can be specified under options[:engine],
either as a Tilt::Template subclass or a name. When not specified,
`_render` infers the engine to use from the `view` parameter when possible.

`render` is promoted in the public API to allow clients to request rendering
while relying on engine inference. The signature has been generalized
accordingly.
1df6fe6
@travisbot

This pull request fails (merged 1df6fe6 into 8752085).

@blambeau blambeau Make sure that options merging occurs in all possible cases.
There is an extra cost for infered engines, because we need two calls
to Tilt[] to guarantee that the global engine options will correctly be
found.
9e4f785
@travisbot

This pull request fails (merged 9e4f785 into 8752085).

@travisbot

This pull request fails (merged e445d00 into 8752085).

@blambeau

@rkh I'm almost done here. A few stuff could still be improved but I'm not sure where to attack exactly... I'll wait for your feedback before going any further. Thanks!

@rkh
Member
rkh commented Jun 15, 2012

I'll go through this once I'm back from Nordic and RuLu. Thanks!

@blambeau

@rkh ping ;-) Any feedback on this?

I don't really care about the code itself. But I'm more and more convinced that, currently, knowing the engine is the responsibility of the caller, whereas IMHO, it should be the secret of the template itself. Through the file extension, as in Tilt. It should also be possible to specify the engine of inline templates at declaration time.

Any thoughts?

@rkh
Member
rkh commented Jun 26, 2012

In general, I agree with you. I have yet to do a code review, though. At first glance, I don't like that there is a _render method.

@blambeau

Great, take your time. It was just to be sure that I can bet on the general idea in the future.

@rkh
Member
rkh commented Feb 26, 2013

I have been thinking about Sinatra 2.0 a lot and wanna redo the whole rendering thing there.

@sdalu
sdalu commented Feb 28, 2013

It would be great if templates could also be chosen according to locales, for example:

erb :view, :locales     => I18n.fallbacks[:'fr-FR'],
           :locales_map => lambda {|relative_path, locale| locale+/+relative_path }

would render the first available :view by looking at files fr-FR/view.erb fr/view.erb en/view.erb

@rkh
Member
rkh commented Feb 28, 2013

While this would indeed be great for some apps, localisation is out of scope for the Sinatra gem itself.

@sdalu
sdalu commented Feb 28, 2013

It's why in my example all the informations about localisation itself is provided as options (so no gem depency with i18n/l10n). All that is needed from sinatra is some very simple logic to iterate over a list of files and choose the first existing one.

Let's say if you prefer without references to localisation:

erb :view, :file_opt      => [ 'a', 'b', 'c' ]
           :file_mangling => lambda {|relative_path, opt| opt+/+relative_path }

would render the first available :view by looking at files a/view.erb b/view.erb c/view.erb

@zzak
Member
zzak commented Feb 28, 2013

@sdalu Please don't hijack someone else's thread and open a separate ticket to request your feature.

Thanks!

@sdalu
sdalu commented Feb 28, 2013

Oops

@JonRowe
JonRowe commented Mar 3, 2013

Great, what does that mean for this pull request? Seems that it's gone a bit stale, can/should it be closed if you're planning on reworking it for Sinatra 2.0?

edit This is in reference to @rkh's original comment,

"I have been thinking about Sinatra 2.0 a lot and wanna redo the whole rendering thing there."
@blambeau
blambeau commented Mar 3, 2013

I think it can simply be closed, provided requested rendering support remains remembered: mostly to be able to ask for rendering based on a file (through its extension).

@JonRowe
JonRowe commented Mar 3, 2013

Doesn't tilt provide that functionality to sinatra already?

@rkh
Member
rkh commented Mar 4, 2013

Yes, vanilla Sinatra just doesn't make use of it.

@rkh rkh closed this Apr 15, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment