T/templates - Githubissues

tommay commented 9 years ago

Two commits:

Add Angelo::Templates which currently supports haml, markdown, and erb but could trivially support anything Tilt supports. The opts[:layout] semantics are from sinatra.
Replace the Angelo::Tilt::ERB with a thin compatibility layer on top of Angelo::Templates. Pretty self-explanatory.

There are no additional tests for the additional template types but erb gets tested via the compatibility layer.

The need for two methods, erb (backwards-compatible) and _erb (sinatra-compatible) is pretty awful. Thoughts are welcome.

tommay commented 9 years ago

By the way this factoring completely sucks. It puts a bunch of class methods in Angelo::Base that should be in an instance of a new TemplateLoader class. But this is simple and can be refactored when the functionality is ok.

tommay commented 9 years ago

I got a pull request merged on Tilt so it caches nil, so when they put out a new tilt gem we can use Tilt::Cache instead rolling our own here.

kenichi commented 9 years ago

@tommay sorry i've not had time to delve into this one yet. i've been traveling and working a ton, hope to go through it on the plane back to portland tomorrow.

tommay commented 9 years ago

No worries.

On Wed, Feb 11, 2015 at 10:03 AM, kenichi nakamura <notifications@github.com

wrote:

@tommay https://github.com/tommay sorry i've not had time to delve into this one yet. i've been traveling and working a ton, hope to go through it on the plane back to portland tomorrow.

— Reply to this email directly or view it on GitHub https://github.com/kenichi/angelo/pull/43#issuecomment-73930763.

kenichi commented 9 years ago

so far, so good. the tilt/erb autoload warning is :/ do you know what specifically causes this? also, since i don't know haml (😁) i tried a markdown one and got a redcarpet load error: should we add these new deps to the gemspec?

tommay commented 9 years ago

gems like haml and redcarpet should be added to your project's Gemfile along with angelo. angelo doesn't rely on them except maybe for future testing but not in general. And there are many markdown gems, you can use whichever you like.

The autoload warning is a good thing because things should be autoloaded at init time because they're right that loading when things are running multi-threaded is no good under MRI at least. The best thing would be to require redcarpet et al before tilt and have tilt load tilt/redcarpet if it sees redcarpet has been loaded. Or vice versa. Which should be possible but hopefully not too hairball.

Having my _erb method is suboptimal. I just did it because I didn't want to overwrite the existing erb method, but I did want to re-implement erb on top of _erb.

On Mon, Feb 16, 2015 at 10:38 AM, kenichi nakamura <notifications@github.com

wrote:

so far, so good. the tilt/erb autoload warning is :/ do you know what specifically causes this? also, since i don't know haml ([image: 😁]) i tried a markdown one and got a redcarpet load error: should we add these new deps to the gemspec?

— Reply to this email directly or view it on GitHub https://github.com/kenichi/angelo/pull/43#issuecomment-74553734.

tommay commented 9 years ago

I'm looking into this warning some more. If tilt knows that tilt/redcarpet should have been required when we try to render some markdown, it sure seems like it could have required it itself when it had the chance and if the requires are ordered properly. I'll see about tweaking tilt.

On Mon, Feb 16, 2015 at 10:56 AM, Tom May tom@tommay.net wrote:

gems like haml and redcarpet should be added to your project's Gemfile along with angelo. angelo doesn't rely on them except maybe for future testing but not in general. And there are many markdown gems, you can use whichever you like.

The autoload warning is a good thing because things should be autoloaded at init time because they're right that loading when things are running multi-threaded is no good under MRI at least. The best thing would be to require redcarpet et al before tilt and have tilt load tilt/redcarpet if it sees redcarpet has been loaded. Or vice versa. Which should be possible but hopefully not too hairball.

Having my _erb method is suboptimal. I just did it because I didn't want to overwrite the existing erb method, but I did want to re-implement erb on top of _erb.

On Mon, Feb 16, 2015 at 10:38 AM, kenichi nakamura < notifications@github.com> wrote:

so far, so good. the tilt/erb autoload warning is :/ do you know what specifically causes this? also, since i don't know haml ([image: 😁]) i tried a markdown one and got a redcarpet load error: should we add these new deps to the gemspec?

— Reply to this email directly or view it on GitHub https://github.com/kenichi/angelo/pull/43#issuecomment-74553734.

tommay commented 9 years ago

FWIW this warning doesn't happen with sinatra because sinatra uses tilt 1.x and angelo uses tilt 2.x.

On Mon, Feb 16, 2015 at 11:43 AM, Tom May tom@tommay.net wrote:

I'm looking into this warning some more. If tilt knows that tilt/redcarpet should have been required when we try to render some markdown, it sure seems like it could have required it itself when it had the chance and if the requires are ordered properly. I'll see about tweaking tilt.

On Mon, Feb 16, 2015 at 10:56 AM, Tom May tom@tommay.net wrote:

gems like haml and redcarpet should be added to your project's Gemfile along with angelo. angelo doesn't rely on them except maybe for future testing but not in general. And there are many markdown gems, you can use whichever you like.

The autoload warning is a good thing because things should be autoloaded at init time because they're right that loading when things are running multi-threaded is no good under MRI at least. The best thing would be to require redcarpet et al before tilt and have tilt load tilt/redcarpet if it sees redcarpet has been loaded. Or vice versa. Which should be possible but hopefully not too hairball.

Having my _erb method is suboptimal. I just did it because I didn't want to overwrite the existing erb method, but I did want to re-implement erb on top of _erb.

On Mon, Feb 16, 2015 at 10:38 AM, kenichi nakamura < notifications@github.com> wrote:

so far, so good. the tilt/erb autoload warning is :/ do you know what specifically causes this? also, since i don't know haml ([image: 😁]) i tried a markdown one and got a redcarpet load error: should we add these new deps to the gemspec?

— Reply to this email directly or view it on GitHub https://github.com/kenichi/angelo/pull/43#issuecomment-74553734.

tommay commented 9 years ago

Ha ha ha, even if you "require 'tilt/haml'" or whatever the tilt code is still not threadsafe since it will still update its @template_map hash the "first" time you render a haml or whatever template, which in general could happen from multiple threads.

Code like that makes me sad.

I'll see what I can do about getting things pre-required and/or registered with tilt for gems that were required before angelo, and most likely open a tilt issue as well. The whole scene is a cluster.

kenichi commented 9 years ago

ok, let's do this

kenichi / angelo

T/templates #43