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

Fix duplicate caching of some templates #1021

Merged
merged 1 commit into from May 9, 2016

Conversation

Projects
None yet
3 participants
@raxoft
Contributor

raxoft commented Aug 3, 2015

The change above makes sure that the default_content_type option is always deleted from the options.

This in turn makes sure that identical template is cached only once, regardless of whether the explicit content_type option is specified or not.

Not to mention it also makes sure that the default_content_type option is not passed as an engine option to the engine which might complain about it.

Fix duplicate caching of some templates
The change above makes sure that the `default_content_type` option is always deleted from the options.
This in turn makes sure that identical template is cached only once, regardless of whether the explicit `content_type` option is specified or not.
Not to mention it also makes sure that the `default_content_type` option is not passed as an engine option to the engine which might complain about it.
@@ -805,7 +805,8 @@ def render(engine, data, options = {}, locals = {}, &block)
layout = engine_options[:layout] if layout.nil? or (layout == true && engine_options[:layout] != false)
layout = @default_layout if layout.nil? or layout == true
layout_options = options.delete(:layout_options) || {}
content_type = options.delete(:content_type) || options.delete(:default_content_type)
content_type = options.delete(:default_content_type)
content_type = options.delete(:content_type) || content_type
layout_engine = options.delete(:layout_engine) || engine
scope = options.delete(:scope) || self
options.delete(:layout)

This comment has been minimized.

@jkowens

jkowens Jan 21, 2016

Member

How about just calling options.delete(:default_content_type) here (L811) instead?

@jkowens

jkowens Jan 21, 2016

Member

How about just calling options.delete(:default_content_type) here (L811) instead?

This comment has been minimized.

@raxoft

raxoft Jan 26, 2016

Contributor

I think the fix is fine as it is, as the relevant lines are all together, but whatever rocks your boat. It's zzak's call in the end after all.

@raxoft

raxoft Jan 26, 2016

Contributor

I think the fix is fine as it is, as the relevant lines are all together, but whatever rocks your boat. It's zzak's call in the end after all.

@zzak zzak added the feedback label Jan 24, 2016

@zzak

This comment has been minimized.

Show comment
Hide comment
@zzak

zzak Jan 24, 2016

Member

Needs a test

Member

zzak commented Jan 24, 2016

Needs a test

@raxoft

This comment has been minimized.

Show comment
Hide comment
@raxoft

raxoft Jan 26, 2016

Contributor

@zzak: I can come up with one, sure. But it won't be anytime soon, if you don't mind. I expect couple of weeks more before I can get back to it...

Contributor

raxoft commented Jan 26, 2016

@zzak: I can come up with one, sure. But it won't be anytime soon, if you don't mind. I expect couple of weeks more before I can get back to it...

@zzak

This comment has been minimized.

Show comment
Hide comment
@zzak

zzak Jan 31, 2016

Member

@raxoft No rush, if you'd like to work on it let me know!

Member

zzak commented Jan 31, 2016

@raxoft No rush, if you'd like to work on it let me know!

@zzak zzak merged commit 1ae202f into sinatra:master May 9, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

zzak added a commit that referenced this pull request May 9, 2016

@zzak

This comment has been minimized.

Show comment
Hide comment
@zzak

zzak May 9, 2016

Member

@raxoft Thank you! I added a regression test in bb40ef6

Member

zzak commented May 9, 2016

@raxoft Thank you! I added a regression test in bb40ef6

@raxoft

This comment has been minimized.

Show comment
Hide comment
@raxoft

raxoft May 12, 2016

Contributor

Thanks!

Contributor

raxoft commented May 12, 2016

Thanks!

@zzak zzak added this to the 2.0.0 milestone Aug 21, 2016

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