Skip to content
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

Merged
merged 1 commit into from Mar 19, 2020

Conversation

@mkaito
Copy link
Contributor

@mkaito mkaito commented Feb 9, 2017

This allows to set render engine options local to a namespace

Fixes #1254

@zzak
Copy link
Member

@zzak zzak commented Mar 4, 2017

This is neat, thank you for the patch!

Let's consider it for after a 2.0 release is made 🙇‍♂️

@zzak zzak added this to the Beyond milestone Mar 4, 2017
@zzak zzak added the feature label Mar 4, 2017
@namusyaka
Copy link
Contributor

@namusyaka namusyaka commented Mar 19, 2017

Could you add a spec for testing this behavior?

@mkaito
Copy link
Contributor Author

@mkaito mkaito commented Mar 20, 2017

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
]

This comment has been minimized.

@namusyaka

namusyaka Mar 20, 2017
Contributor

Allowed keys should be defined as a constant.

This comment has been minimized.

@olleolleolle

olleolleolle Aug 18, 2017
Member

@mkaito Example constant names: ALLOWED_RENDER_ENGINE_KEYS, RENDER_ENGINE_NAMES, et cetera.

@olleolleolle
Copy link
Member

@olleolleolle olleolleolle commented Aug 18, 2017

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
@namusyaka namusyaka modified the milestones: Beyond, v2.0.2 Feb 19, 2018
@namusyaka
Copy link
Contributor

@namusyaka namusyaka commented Mar 6, 2018

@mkaito Hey, could you still address on this? If not, another patch is welcome.

@mkaito
Copy link
Contributor Author

@mkaito mkaito commented Mar 6, 2018

@namusyaka
Copy link
Contributor

@namusyaka namusyaka commented Mar 6, 2018

Don't worry. Just listening to that reply is enough.
Thanks.

@namusyaka namusyaka modified the milestones: v2.0.2, v2.0.3, v2.0.4 Jun 5, 2018
@namusyaka namusyaka modified the milestones: v2.0.4, v2.0.5 Sep 14, 2018
@namusyaka namusyaka modified the milestones: v2.0.5, v2.0.6 Dec 22, 2018
@jkowens jkowens modified the milestones: v2.0.6, v2.1.0 Mar 14, 2020
@jkowens jkowens force-pushed the mkaito:mkaito/namespace/allow-set-engine branch 2 times, most recently from 97170f2 to 7de9bd2 Mar 18, 2020
@jkowens jkowens requested a review from olleolleolle Mar 18, 2020
Copy link
Member

@olleolleolle olleolleolle left a comment

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
@jkowens jkowens force-pushed the mkaito:mkaito/namespace/allow-set-engine branch from 7de9bd2 to 8c53a79 Mar 19, 2020
@jkowens jkowens merged commit 2850541 into sinatra:master Mar 19, 2020
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@olleolleolle
Copy link
Member

@olleolleolle olleolleolle commented Mar 19, 2020

@jkowens Hooray! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

5 participants
You can’t perform that action at this time.