Smart Templates #70

Merged
2 commits merged into from Oct 9, 2010

Projects

None yet

2 participants

@rkh
Sinatra member

Improved templates a bit:

  • Template engines now set default content type (i.e. using sass will default to css rather than html). Content types can still be set explicitly and all corner cases should be covered.
  • Nested templates don't use implicit layouts anymore. Eases the use of rendering methods for partials.

Those are two separate features, but applying the first without the latter to master results in a merge conflict that does not need to be resolved in case we decide to merge both.

Tests are, of course, included.

rkh added some commits Sep 19, 2010
@rkh rkh Skip implicit layouts for nested templates.
That way the following will produce valid HTML:

@@ layout
!!!
= yield

@@ content
%html
  %head= haml :head
  %body= haml :body

That way using render methods for partials is a lot easier.
Tests included.
e5e0047
@rkh rkh Sets default content type according to template engine used instead o…
…f just text/html.

It does so by including a Mixin into the the returned string offering a content_type method. Therefore all of the following examples produce the expected results:

    # text/html
    get('/') do
      haml :index
    end

    # text/css
    get('/') do
      sass :index
    end

    # text/css
    get('/') do
      haml :index
      sass :index
    end

    # text/html
    get('/') do
      haml '= sass :index'
    end

It also allows setting the default content type for a template engine:

    set :builder, :content_type => :html

Tests and README adjustments (all languages) included.
1d676f4
@rkh
Sinatra member
rkh commented Oct 9, 2010

Since there is no feedback so far and these are just minor changes (no real API additions), I'll merge it into master. If there are any complains, I will revert the changes.

@ryansobol

Huh? Is this really the best way of adding a content_type accessor to a String instance (I presume) ? +1 clever points at least. :P

Sinatra member

It avoids tons of edge cases (what if you nest different template engines, what if you call haml just for fun and return something different instead). Even though it doesn't seem so at first glance, this is rather noninvasive and OO. By extending output with a mixin we avoid monkey-patching plus get better documentation. But if you have another approach in mind, I'd love to wrap my head around it.

Your rationale seems sound, although I'm still wrestling how we get "better documentation" with this technique. I guess it makes sense if you're referring exclusively to Sinatra contributors. I might have gone with a different technique, encapsulating output and it's meta-data in a first-class object. But this surely would have lead to more code, more tests, more potential bugs, and more time. Your technique is quick and precise, which is essential for the next release.

BTW - Konstantin, you've done a f*cking amazing job with organizing this 1.1 release. I'm really excited about smart templates. When I upgrade Sinatra in my project, I'm going to delete so much code! And my users are going to love having more template language choices! I hope you don't mind a little peer-reviewing from me.

Sinatra member

Thanks. No, it's fine, welcomed even. The reason for not creating a own class is that we had to cover every possibility where such a class could be handed to rack, including using it for streaming (wrapping it in another project) or with tools like async-sinatra, as Rack expects strings. Returning something that just behaves like a string would violate the Rack spec. With documentation I mean as opposed to monkey-patching the string directly. The mixin can actually show up in generated documentation (not that it matters much without comments, but it's a start). If you got more feedback/discussion, keep it coming.

It's no question that compatibility with the Rack spec is paramount (for all Ruby web frameworks). And I would never advocate violating that. Using the technique of a wrapper class, as I've hinted, would require more work to ensure compatibility with Rack (and with async-sinatra for that matter). Right now, I don't think it's worth the time investment.

As far as improving Sinatra's documentation, I wish I had more time to pitch in. :( It's a good thing the source is so readable already. :P I've actually added quite a few techniques into my own toolbox from periodically perusing the source.

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment