Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Explicitly set falsy values for :template should not render a template #628

Merged
merged 1 commit into from

2 participants

@richo

No description provided.

@rkh
Owner
rkh commented

This would change the behaviour of the :layout option and might break apps and especially extensions. It also influences eat_errors (see line below). What's the use case?

@richo

Well, would it? I checked that all the specs are passing and this only affects the behaviour when nil is explicitly set. It doesn't affect the value of eat_errors, because it tests for existance in the opts hash.

The usecase is my utter confusion when I set the :layout parameter to nil and still got a layout. My (I feel reasonable) expectation is that setting either of the false values in ruby should opt out.

@richo

Any news on this? I think it's the right behaviour for a reasonable set of expectations but open to discussion.

@rkh
Owner
rkh commented

Sorry, yes, I'm travelling atm.

@richo

Ah, no stress.

You got caught up in my periodic bump of all my pull requests. Speak to you when you're available :)

@richo

Bamp.

@rkh rkh commented on the diff
lib/sinatra/base.rb
@@ -714,13 +714,15 @@ def render(engine, data, options={}, locals={}, &block)
# extract generic options
locals = options.delete(:locals) || locals || {}
views = options.delete(:views) || settings.views || "./views"
- layout = options.delete(:layout)
+ layout = options[:layout]
+ layout = false if layout.nil? && options.include?(:layout)
@rkh Owner
rkh added a note

This messes with the line right beneath it.

@richo
richo added a note

Can you elaberate on how? eat_errors was previously true if the default value (nil) was used. This just specialcases where you've specified nil.

Can you show me a testcase for what you mean, and I'll fix it.

@rkh Owner
rkh added a note

It seems like we don't have a test for this yet. But given the code eat_errors will always be false.

@rkh Owner
rkh added a note

Ah, no, sorry, missed the include? part.

@richo
richo added a note

Yeah I'm pretty sure there are tests over it. I'll open a PR with some explainatory comments if you want, if it took you a couple of goes to work out what I'm doing it'll probably be an issue again in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@rkh rkh merged commit 2fc53ac into sinatra:master
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
Showing with 19 additions and 2 deletions.
  1. +4 −2 lib/sinatra/base.rb
  2. +15 −0 test/templates_test.rb
View
6 lib/sinatra/base.rb
@@ -580,7 +580,7 @@ def with_params(temp_params)
#
# Possible options are:
# :content_type The content type to use, same arguments as content_type.
- # :layout If set to false, no layout is rendered, otherwise
+ # :layout If set to something falsy, no layout is rendered, otherwise
# the specified layout is used (Ignored for `sass` and `less`)
# :layout_engine Engine to use for rendering the layout.
# :locals A hash with local variables that should be available
@@ -714,13 +714,15 @@ def render(engine, data, options={}, locals={}, &block)
# extract generic options
locals = options.delete(:locals) || locals || {}
views = options.delete(:views) || settings.views || "./views"
- layout = options.delete(:layout)
+ layout = options[:layout]
+ layout = false if layout.nil? && options.include?(:layout)
@rkh Owner
rkh added a note

This messes with the line right beneath it.

@richo
richo added a note

Can you elaberate on how? eat_errors was previously true if the default value (nil) was used. This just specialcases where you've specified nil.

Can you show me a testcase for what you mean, and I'll fix it.

@rkh Owner
rkh added a note

It seems like we don't have a test for this yet. But given the code eat_errors will always be false.

@rkh Owner
rkh added a note

Ah, no, sorry, missed the include? part.

@richo
richo added a note

Yeah I'm pretty sure there are tests over it. I'll open a PR with some explainatory comments if you want, if it took you a couple of goes to work out what I'm doing it'll probably be an issue again in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
eat_errors = layout.nil?
layout = engine_options[:layout] if layout.nil? or layout == true
layout = @default_layout if layout.nil? or layout == true
content_type = options.delete(:content_type) || options.delete(:default_content_type)
layout_engine = options.delete(:layout_engine) || engine
scope = options.delete(:scope) || self
+ options.delete(:layout)
# set some defaults
options[:outvar] ||= '@_out_buf'
View
15 test/templates_test.rb
@@ -64,6 +64,21 @@ def with_default_layout
assert_equal "Layout!!! Hello World!", body
end
+ it 'renders no layout if layout if falsy' do
+ mock_app do
+ template(:layout) { 'Layout!!! <%= yield %>' }
+ set :erb, :layout => true
+
+ get('/') do
+ erb('Hello World!', { :layout => nil })
+ end
+ end
+
+ get '/'
+ assert ok?
+ assert_equal "Hello World!", body
+ end
+
it 'renders String templates directly' do
render_app { render(:test, 'Hello World') }
assert ok?
Something went wrong with that request. Please try again.