-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
sinatra-namespace: Allow for set :<engine>
#1255
sinatra-namespace: Allow for set :<engine>
#1255
Conversation
This is neat, thank you for the patch! Let's consider it for after a 2.0 release is made 🙇♂️ |
Could you add a spec for testing this behavior? |
I don't even know where to begin with that. I'll try, though. |
:erb, :haml, :builder, :nokogiri, :sass, :scss, :less, :liquid, :markdown, | ||
:textile, :rdoc, :asciidoc, :radius, :markaby, :rabl, :slim, :creole, | ||
:mediawiki, :coffee, :stylus, :yajl, :wlang | ||
] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Allowed keys should be defined as a constant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mkaito Example constant names: ALLOWED_RENDER_ENGINE_KEYS
, RENDER_ENGINE_NAMES
, et cetera.
Here's a spec which explains the behaviour: diff --git a/sinatra-contrib/spec/namespace_spec.rb b/sinatra-contrib/spec/namespace_spec.rb
index 15381560..be06370a 100644
--- a/sinatra-contrib/spec/namespace_spec.rb
+++ b/sinatra-contrib/spec/namespace_spec.rb
@@ -818,5 +818,15 @@ describe Sinatra::Namespace do
get '/foo-bar'
expect(last_response.body).to eq('[:foo_bar]')
end
+
+ it 'forbids unknown engine settings' do
+ expect {
+ mock_app do
+ namespace '/foo' do
+ set :unknownsetting
+ end
+ end
+ }.to raise_error(ArgumentError, 'may not set unknownsetting')
+ end
end
end
|
@mkaito Hey, could you still address on this? If not, another patch is welcome. |
I... Completely forgot about this. Sorry. Yes, I'll look at it again as soon as I have a little time.
El 6 de marzo de 2018 13:13:14 GMT+00:00, namusyaka <notifications@github.com> escribió:
…
@mkaito Hey, could you still address on this? If not, another patch is
welcome.
--
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
#1255 (comment)
--
Enviado desde mi dispositivo Android con K-9 Mail. Por favor, disculpa mi brevedad.
|
Don't worry. Just listening to that reply is enough. |
97170f2
to
7de9bd2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this looks great and the test explains what happens when used wrong.
Let's get this merged!
This allows to set render engine options local to a namespace
7de9bd2
to
8c53a79
Compare
@jkowens Hooray! 🎉 |
This allows to set render engine options local to a namespace
Fixes #1254