Loading extensions before Padrino::Rendering #541

Closed
nesquena opened this Issue May 22, 2011 · 45 comments

Comments

Projects
None yet
4 participants
@nesquena
Member

nesquena commented May 22, 2011

Overflow from #534 . What should we do about this? I.E

@botanicus:

Thanks a lot, I'm trying to take a look, unfortunately on Padrino HEAD I have a different problem, not really a bug, but an implementation thing which makes it impossible to register first TemplateInheritance::Rendering and then Padrino::Rendering ... I'm just on IRC asking for help, once this will be addressed, I'll check if this work. Thanks for a very quick response!

@nesquena:

yes, exactly. May I suggest to make registering Padrino::Rendering (currently in padrino-core/application.rb) user responsibility? So when a user would generate a new app, he would get http://pastie.org/1928277 Apart of that that I believe is better solution (not everyone needs rendering really), it'd easily solve my problem with registering modules in the right order. Or to have some base class, sth like Padrino::BaseApplication.

Basically he wants to hook into render before the Padrino rendering module is loaded. How do we address this?

@ghost ghost assigned DAddYE May 22, 2011

@nesquena

This comment has been minimized.

Show comment
Hide comment
@nesquena

nesquena May 22, 2011

Member

@DAddYE Should we deal with this in this release or push to next?

Member

nesquena commented May 22, 2011

@DAddYE Should we deal with this in this release or push to next?

@DAddYE

This comment has been minimized.

Show comment
Hide comment
@DAddYE

DAddYE May 22, 2011

Member

Next

Member

DAddYE commented May 22, 2011

Next

@nesquena

This comment has been minimized.

Show comment
Hide comment
@nesquena

nesquena May 22, 2011

Member

Ok figured...

Member

nesquena commented May 22, 2011

Ok figured...

@botanicus

This comment has been minimized.

Show comment
Hide comment
@botanicus

botanicus May 22, 2011

Contributor

If I may, the adapter for Padrino in template-inheritance works in 0.9.28 and regardless of implementation details, I'd really appreciate if it could work in 0.9.29 as well. Otherwise it'd be me who'll get bug reports about not compatible plugin and I'd have to "if" for Padrino versions and do something else for 0.9.29 (not that I'd know how to fix it on current Padrino HEAD, but I'd have to do something about it ... ), so please guys, if you can, fix it now. Cheers.

Contributor

botanicus commented May 22, 2011

If I may, the adapter for Padrino in template-inheritance works in 0.9.28 and regardless of implementation details, I'd really appreciate if it could work in 0.9.29 as well. Otherwise it'd be me who'll get bug reports about not compatible plugin and I'd have to "if" for Padrino versions and do something else for 0.9.29 (not that I'd know how to fix it on current Padrino HEAD, but I'd have to do something about it ... ), so please guys, if you can, fix it now. Cheers.

@DAddYE

This comment has been minimized.

Show comment
Hide comment
@DAddYE

DAddYE May 22, 2011

Member

Ok, @nesquena, can u do that?

Member

DAddYE commented May 22, 2011

Ok, @nesquena, can u do that?

@nesquena

This comment has been minimized.

Show comment
Hide comment
@nesquena

nesquena May 22, 2011

Member

@DAddYE, what do you recommend we do about this? I'm not sure the best way to approach solving this or why it worked in 0.9.28? Will move it back for now

Member

nesquena commented May 22, 2011

@DAddYE, what do you recommend we do about this? I'm not sure the best way to approach solving this or why it worked in 0.9.28? Will move it back for now

@DAddYE

This comment has been minimized.

Show comment
Hide comment
@DAddYE

DAddYE May 22, 2011

Member

A momentan patch:

  class Application < Sinatra::Base
    register Padrino::Routing
    register Padrino::Rendering unless defined?(SKIP_PADRINO_RENDERING)
Member

DAddYE commented May 22, 2011

A momentan patch:

  class Application < Sinatra::Base
    register Padrino::Routing
    register Padrino::Rendering unless defined?(SKIP_PADRINO_RENDERING)
@nesquena

This comment has been minimized.

Show comment
Hide comment
@nesquena

nesquena May 22, 2011

Member

You want us to add in that hack patch and then botanicus would set the constant in his extension? I guess that could work, seems a bit of a hack though, don't you think? Maybe we should move the register's down later and then check an option? Anyway we can achieve:

class Demo < Padrino::Application
  disable :padrino_rendering
  register TemplateInheritance::Rendering
  register Padrino::Rendering
end

What do you think, is that easily possible? A good idea?

Member

nesquena commented May 22, 2011

You want us to add in that hack patch and then botanicus would set the constant in his extension? I guess that could work, seems a bit of a hack though, don't you think? Maybe we should move the register's down later and then check an option? Anyway we can achieve:

class Demo < Padrino::Application
  disable :padrino_rendering
  register TemplateInheritance::Rendering
  register Padrino::Rendering
end

What do you think, is that easily possible? A good idea?

@DAddYE

This comment has been minimized.

Show comment
Hide comment
@DAddYE

DAddYE May 22, 2011

Member

my patch is super secure.

This need to be a bit tested because we need to move Padrino::Rendering in register_initializers

Member

DAddYE commented May 22, 2011

my patch is super secure.

This need to be a bit tested because we need to move Padrino::Rendering in register_initializers

@nesquena

This comment has been minimized.

Show comment
Hide comment
@nesquena

nesquena May 22, 2011

Member

Fair enough, I tried my way and there were still some failing tests. This way seems a bit hacky but it is OK for now: a9dc6f3

Now to get it to work just have to do:

SKIP_PADRINO_RENDERING = true

before the creation of the Padrino application and then we would need to put:

class Demo < Padrino::Application
  register TemplateInheritance::Rendering
  register Padrino::Rendering
end

Despite being a bit hacky, does this work for now @DAddYE @botanicus?

Member

nesquena commented May 22, 2011

Fair enough, I tried my way and there were still some failing tests. This way seems a bit hacky but it is OK for now: a9dc6f3

Now to get it to work just have to do:

SKIP_PADRINO_RENDERING = true

before the creation of the Padrino application and then we would need to put:

class Demo < Padrino::Application
  register TemplateInheritance::Rendering
  register Padrino::Rendering
end

Despite being a bit hacky, does this work for now @DAddYE @botanicus?

@DAddYE

This comment has been minimized.

Show comment
Hide comment
@DAddYE

DAddYE May 22, 2011

Member

We have few time right now and I agree is horrible we will try to do something better in future releases.

Member

DAddYE commented May 22, 2011

We have few time right now and I agree is horrible we will try to do something better in future releases.

@DAddYE

This comment has been minimized.

Show comment
Hide comment
@DAddYE

DAddYE May 22, 2011

Member

Nate:

def before_render(&:block)
  (@_before_render ||= []) << block
end

def render( ... )
  @_before_render.each(&:call)
end
Member

DAddYE commented May 22, 2011

Nate:

def before_render(&:block)
  (@_before_render ||= []) << block
end

def render( ... )
  @_before_render.each(&:call)
end
@DAddYE

This comment has been minimized.

Show comment
Hide comment
@DAddYE

DAddYE May 22, 2011

Member

@botanicus can do:

Padrino::Application.before_render do
   ... do some ...
end
Member

DAddYE commented May 22, 2011

@botanicus can do:

Padrino::Application.before_render do
   ... do some ...
end
@botanicus

This comment has been minimized.

Show comment
Hide comment
@botanicus

botanicus May 22, 2011

Contributor

SKIP_PADRINO_RENDERING is great, it's hacky but it works and it's what matters. BTW if we are postponing it, then maybe we can drop registering of Padrino::Rendering in the next bigger release :) ? Just a suggestion, I don't want to interfere, but you know my opinion on registering rendering by default and I believe it would make things easier.

Contributor

botanicus commented May 22, 2011

SKIP_PADRINO_RENDERING is great, it's hacky but it works and it's what matters. BTW if we are postponing it, then maybe we can drop registering of Padrino::Rendering in the next bigger release :) ? Just a suggestion, I don't want to interfere, but you know my opinion on registering rendering by default and I believe it would make things easier.

@botanicus

This comment has been minimized.

Show comment
Hide comment
@botanicus

botanicus May 22, 2011

Contributor

@DAddYE sorry, I'm not following, how could before_render help?

Contributor

botanicus commented May 22, 2011

@DAddYE sorry, I'm not following, how could before_render help?

@nesquena

This comment has been minimized.

Show comment
Hide comment
@nesquena

nesquena May 22, 2011

Member

I think he was going for

Padrino::Application.before_render do
  register TemplateInheritance::Rendering
end

Right @DAddYE? I think this is actually more confusing naming and more work then the constant. Maybe in a future version we can figure out a better approach but I am OK with hacky SKIP_PADRINO_RENDERING constant for now.

@botanicus I don't have any problem with removing the auto register of Padrino::Rendering except for that it would break every single Padrino application in previous versions and it would not be obvious how to fix it unless you were careful to read a blog post. Maybe if we could get the console to print out a suggestion to include it when the error occurs...

Member

nesquena commented May 22, 2011

I think he was going for

Padrino::Application.before_render do
  register TemplateInheritance::Rendering
end

Right @DAddYE? I think this is actually more confusing naming and more work then the constant. Maybe in a future version we can figure out a better approach but I am OK with hacky SKIP_PADRINO_RENDERING constant for now.

@botanicus I don't have any problem with removing the auto register of Padrino::Rendering except for that it would break every single Padrino application in previous versions and it would not be obvious how to fix it unless you were careful to read a blog post. Maybe if we could get the console to print out a suggestion to include it when the error occurs...

@DAddYE

This comment has been minimized.

Show comment
Hide comment
@DAddYE

DAddYE May 22, 2011

Member

Ok, for the constant!

Member

DAddYE commented May 22, 2011

Ok, for the constant!

@nesquena

This comment has been minimized.

Show comment
Hide comment
@nesquena

nesquena May 22, 2011

Member

@DAddYE @botanicus I am thinking about it, maybe for 0.10.0 we could remove the auto registration of Render. I mean it is a point release finally, if we were ever going to do it, then it would be here. We did the same thing for Padrino::Helpers and I think it is a good thing to allow people to strip away functionality they don't need. Demonstrates how modular our system is...

Member

nesquena commented May 22, 2011

@DAddYE @botanicus I am thinking about it, maybe for 0.10.0 we could remove the auto registration of Render. I mean it is a point release finally, if we were ever going to do it, then it would be here. We did the same thing for Padrino::Helpers and I think it is a good thing to allow people to strip away functionality they don't need. Demonstrates how modular our system is...

@nesquena

This comment has been minimized.

Show comment
Hide comment
@nesquena

nesquena May 22, 2011

Member

Moving this ticket to 0.10 since SKIP_PADRINO_RENDERING works for now.

Member

nesquena commented May 22, 2011

Moving this ticket to 0.10 since SKIP_PADRINO_RENDERING works for now.

@DAddYE

This comment has been minimized.

Show comment
Hide comment
@DAddYE

DAddYE May 22, 2011

Member

My employes need to edit thousand sites, but 100% agree.

Member

DAddYE commented May 22, 2011

My employes need to edit thousand sites, but 100% agree.

@nesquena

This comment has been minimized.

Show comment
Hide comment
@nesquena

nesquena May 22, 2011

Member

Great then for 0.10.0 then barring any serious issues not to, we can remove the Padrino::Render and move into the generator. I think this is a good idea going forward and again demonstrates the powerful (take what you need) modularity of our system.

Member

nesquena commented May 22, 2011

Great then for 0.10.0 then barring any serious issues not to, we can remove the Padrino::Render and move into the generator. I think this is a good idea going forward and again demonstrates the powerful (take what you need) modularity of our system.

@botanicus

This comment has been minimized.

Show comment
Hide comment
@botanicus

botanicus May 22, 2011

Contributor

Great, cheers a lot guys! BTW there's a clever functionality of RubyGems, Gem::Specification#post_install_message=(msg) (i. e. https://github.com/botanicus/ace/blob/master/ace.gemspec#L48), which allows us to set a message which is displayed once someone installs the gem. I'm using it for printing CHANGELOG using the changelog gem (i. e. https://skitch.com/botanicus/fy246/rubygems-post-install-message), so we can use it to tell users to register rendering themselves. Easy-peasy and with using the right colours no one can miss it ;)

Contributor

botanicus commented May 22, 2011

Great, cheers a lot guys! BTW there's a clever functionality of RubyGems, Gem::Specification#post_install_message=(msg) (i. e. https://github.com/botanicus/ace/blob/master/ace.gemspec#L48), which allows us to set a message which is displayed once someone installs the gem. I'm using it for printing CHANGELOG using the changelog gem (i. e. https://skitch.com/botanicus/fy246/rubygems-post-install-message), so we can use it to tell users to register rendering themselves. Easy-peasy and with using the right colours no one can miss it ;)

@nesquena

This comment has been minimized.

Show comment
Hide comment
@nesquena

nesquena May 22, 2011

Member

Good suggestion, maybe we should print a post-install message for 0.10 explaining the issue and pointing to the blog post... Thanks botanicus for helping us identify and simplify the register process even more.

Member

nesquena commented May 22, 2011

Good suggestion, maybe we should print a post-install message for 0.10 explaining the issue and pointing to the blog post... Thanks botanicus for helping us identify and simplify the register process even more.

@nesquena

This comment has been minimized.

Show comment
Hide comment
@nesquena

nesquena May 23, 2011

Member

@DAddYE Created a branch here: df833ca with Padrino::Render becoming explicit. Look ok?

Member

nesquena commented May 23, 2011

@DAddYE Created a branch here: df833ca with Padrino::Render becoming explicit. Look ok?

@DAddYE

This comment has been minimized.

Show comment
Hide comment
@DAddYE

DAddYE May 23, 2011

Member

Yesse!

Member

DAddYE commented May 23, 2011

Yesse!

@nesquena

This comment has been minimized.

Show comment
Hide comment
@nesquena

nesquena May 23, 2011

Member

@botanicus Thanks for the post install message tip, added one of those as well. I think the patches in that rendering-remove would close this issue, right?

Member

nesquena commented May 23, 2011

@botanicus Thanks for the post install message tip, added one of those as well. I think the patches in that rendering-remove would close this issue, right?

@botanicus

This comment has been minimized.

Show comment
Hide comment
@botanicus

botanicus May 23, 2011

Contributor

@nesquena amazing, that's all I need :) ! Cheers!

Contributor

botanicus commented May 23, 2011

@nesquena amazing, that's all I need :) ! Cheers!

@nesquena

This comment has been minimized.

Show comment
Hide comment
@nesquena

nesquena May 23, 2011

Member

Great, will close this once we can merge the branch into master

Member

nesquena commented May 23, 2011

Great, will close this once we can merge the branch into master

@DAddYE

This comment has been minimized.

Show comment
Hide comment
@DAddYE

DAddYE Jun 5, 2011

Member

@nesquena I think we are ready to add this. Can u do that?

Member

DAddYE commented Jun 5, 2011

@nesquena I think we are ready to add this. Can u do that?

@nesquena

This comment has been minimized.

Show comment
Hide comment
@nesquena

nesquena Jun 6, 2011

Member

Yeah I will soon.

Member

nesquena commented Jun 6, 2011

Yeah I will soon.

@DAddYE

This comment has been minimized.

Show comment
Hide comment
@DAddYE

DAddYE Jun 7, 2011

Member

@nesquena can u do that before the release?

Member

DAddYE commented Jun 7, 2011

@nesquena can u do that before the release?

@nesquena

This comment has been minimized.

Show comment
Hide comment
@nesquena

nesquena Jun 7, 2011

Member

I'd like to do this in the subsequent release along with locking to Sinatra 1.3 once that is released (soon). Unless that's released in next few days and then we can incorporate that to this release.

Member

nesquena commented Jun 7, 2011

I'd like to do this in the subsequent release along with locking to Sinatra 1.3 once that is released (soon). Unless that's released in next few days and then we can incorporate that to this release.

@nesquena

This comment has been minimized.

Show comment
Hide comment
@nesquena

nesquena Jun 8, 2011

Member

Merged into master now. Can you guys confirm it works and post install message is OK @DAddYE @botanicus

Member

nesquena commented Jun 8, 2011

Merged into master now. Can you guys confirm it works and post install message is OK @DAddYE @botanicus

@nesquena

This comment has been minimized.

Show comment
Hide comment
@nesquena

nesquena Jun 8, 2011

Member

Does this work for you guys? Can we close?

Member

nesquena commented Jun 8, 2011

Does this work for you guys? Can we close?

@botanicus

This comment has been minimized.

Show comment
Hide comment
@botanicus

botanicus Jun 9, 2011

Contributor

It does work. Cheers guys!

Contributor

botanicus commented Jun 9, 2011

It does work. Cheers guys!

@nesquena

This comment has been minimized.

Show comment
Hide comment
@nesquena

nesquena Jun 9, 2011

Member

Ok thanks! closing...

Member

nesquena commented Jun 9, 2011

Ok thanks! closing...

@nesquena nesquena closed this Jun 9, 2011

@bernerdschaefer

This comment has been minimized.

Show comment
Hide comment
@bernerdschaefer

bernerdschaefer Jul 1, 2011

Contributor

I wonder if there's another way this can / should be communicated, since bundler does not show post install messages. It took a lot of digging to figure out what had changed between 0.9.29 and master before I found this issue!

Contributor

bernerdschaefer commented Jul 1, 2011

I wonder if there's another way this can / should be communicated, since bundler does not show post install messages. It took a lot of digging to figure out what had changed between 0.9.29 and master before I found this issue!

@nesquena

This comment has been minimized.

Show comment
Hide comment
@nesquena

nesquena Jul 1, 2011

Member

Excellent point, that does strike me as a problem. We need to figure out a way to communicate this more effectively. @DAddYE What do you think?

Member

nesquena commented Jul 1, 2011

Excellent point, that does strike me as a problem. We need to figure out a way to communicate this more effectively. @DAddYE What do you think?

@DAddYE

This comment has been minimized.

Show comment
Hide comment
@DAddYE

DAddYE Jul 1, 2011

Member

We can add a the same warning into padrino-core.rb class? Some like:

puts colored_warning unless App.respond_to?(Rendering) && !App.respond_to?(:skip_rendering) # last for those that don't wanna see it
Member

DAddYE commented Jul 1, 2011

We can add a the same warning into padrino-core.rb class? Some like:

puts colored_warning unless App.respond_to?(Rendering) && !App.respond_to?(:skip_rendering) # last for those that don't wanna see it

@nesquena nesquena reopened this Jul 1, 2011

@nesquena

This comment has been minimized.

Show comment
Hide comment
@nesquena

nesquena Jul 1, 2011

Member

Yeah I think a warning that prints in the app itself would be more helpful, good idea

Member

nesquena commented Jul 1, 2011

Yeah I think a warning that prints in the app itself would be more helpful, good idea

@botanicus

This comment has been minimized.

Show comment
Hide comment
@botanicus

botanicus Jul 1, 2011

Contributor

What does App.respond_to?(Rendering) mean? I'd check for instance method render, so it's agnostic to any rendering mechanism.

Contributor

botanicus commented Jul 1, 2011

What does App.respond_to?(Rendering) mean? I'd check for instance method render, so it's agnostic to any rendering mechanism.

@nesquena

This comment has been minimized.

Show comment
Hide comment
@nesquena

nesquena Jul 6, 2011

Member

Ok I took a relatively safe approach here: bb73d92 and basically lazy load this rendering module (with a deprecation notice) when I detect the rendering exception is about to occur. If the user a) never uses the render method with one argument and b) the rendering module has not been included then the message will print and the module is loaded. Can you guys take a look and basically tell me if that seems OK. I am going to hold the release until I get an OK @DAddYE @botanicus . I figure most cases the error will occur like this:

class Demo < Padrino::Application
  # no inclusion of rendering module

  get "/" do
    render "foo" # uses padrino style rendering
  end
end
Member

nesquena commented Jul 6, 2011

Ok I took a relatively safe approach here: bb73d92 and basically lazy load this rendering module (with a deprecation notice) when I detect the rendering exception is about to occur. If the user a) never uses the render method with one argument and b) the rendering module has not been included then the message will print and the module is loaded. Can you guys take a look and basically tell me if that seems OK. I am going to hold the release until I get an OK @DAddYE @botanicus . I figure most cases the error will occur like this:

class Demo < Padrino::Application
  # no inclusion of rendering module

  get "/" do
    render "foo" # uses padrino style rendering
  end
end
@botanicus

This comment has been minimized.

Show comment
Hide comment
@botanicus

botanicus Jul 6, 2011

Contributor

So if he includes my rendering module, he'll get render from there, otherwise he'll get this. Yes, that does make perfect sense.

Contributor

botanicus commented Jul 6, 2011

So if he includes my rendering module, he'll get render from there, otherwise he'll get this. Yes, that does make perfect sense.

@nesquena

This comment has been minimized.

Show comment
Hide comment
@nesquena

nesquena Jul 6, 2011

Member

That was the intention, yes. Do you guys think this will work alright for the release?

Member

nesquena commented Jul 6, 2011

That was the intention, yes. Do you guys think this will work alright for the release?

@nesquena

This comment has been minimized.

Show comment
Hide comment
@nesquena

nesquena Jul 7, 2011

Member

Alright tested and closing

Member

nesquena commented Jul 7, 2011

Alright tested and closing

@nesquena nesquena closed this Jul 7, 2011

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