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: cannot set :layout #1254

Closed
mkaito opened this issue Feb 9, 2017 · 8 comments · Fixed by #1255
Closed

sinatra-namespace: cannot set :layout #1254

mkaito opened this issue Feb 9, 2017 · 8 comments · Fixed by #1255

Comments

@mkaito
Copy link

mkaito commented Feb 9, 2017

What's the logic behind the check at line 282? It seems you can, in fact, only set :views inside a namespace.

I'd like to set the layout for an entire namespace, in this case a CMS-like backend. And I'd like to do so without having to pass the same :layout argument to the erb method every time.

EDIT: Apparently set :layout is not a thing at all. Perhaps it should?
EDIT EDIT: Seems to be set :erb, :layout => :something instead, to set the default layout. But that has sinatra-namespace complaining about set :erb, which brings me back to the question above.

@mwpastore
Copy link
Member

mwpastore commented Feb 9, 2017

set :erb isn't a thing. You'll have to pass the layout each time. You can wrap the ERB helper like this (untested):

namespace '/cms' do
  helpers do
    def erb(template, options={}, locals={}, &block)
      options[:layout] ||= :something
      super(template, options, locals, &block)
    end
  end

  get '' do
    erb :dashboard
  end
end

@mkaito
Copy link
Author

mkaito commented Feb 9, 2017

Oh, but it is a thing. See this right here. You can set :<engine>, <options_hash> and it checks for the :layout option here.

@mwpastore
Copy link
Member

mwpastore commented Feb 9, 2017

Ah, it's not an official setting, but you're right, render will use it if it exists. Good catch.

I don't know why almost all settings are prohibited in namespaces. I've run into this issue myself and the workarounds are... clumsy. Perhaps someone else can chime in and explain the historical reasoning behind this decision.

@mkaito
Copy link
Author

mkaito commented Feb 9, 2017

Without changing the code in sinatra-namespace, and to resolve this, I can set :views to different things depending on the namespace. Some template duplication aside (does tilt play well with symlinks, you think?), it should solve my use case decently.

The question about only allowing set :views still stands though. I'm not familiar enough with Sinatra internals to know whether there are any actual issues, and sinatra-contrib has some ancient code by people that haven't been around for years, so who knows. Does this stuff have any tests?

@mwpastore
Copy link
Member

Is the erb helper solution not an option for some reason? Tilt should be fine with symlinks, but Git (and other version control systems) not so much.

Agreed about sinatra-contrib. I and other contributors have been trying to chip away at some of the bit-rot over the past months. Code coverage is pretty good, all things considered.

@mkaito
Copy link
Author

mkaito commented Feb 9, 2017

Is the erb helper solution not an option for some reason?

No, it'd work just fine. It's just very clumsy. Not anyone's fault, since the code we're working around is just a little weird.

Tilt should be fine with symlinks, but Git (and other version control systems) not so much.

Agreed, I just looked into it, and git doesn't handle project-relative symlinks in any way, which wouldn't work unless everyone stores their git clones in the same path.

Agreed about sinatra-contrib. I and other contributors have been trying to chip away at some of the bit-rot over the past months. Code coverage is pretty good, all things considered.

I might see if I can just "expand" the allowed settings and see if anything breaks. It might just be fine, who knows.

@mkaito
Copy link
Author

mkaito commented Feb 9, 2017

I just did a minor refactor of that section, and all tests still pass. I'm putting up a PR so others can give it a test drive.

@mkaito
Copy link
Author

mkaito commented Feb 9, 2017

Done in #1255

@namusyaka namusyaka added this to the v2.0.2 milestone Feb 19, 2018
@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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants