Extensible Formats System

[lilith]

Well, I've been using this branch for about 10 months now and it seems pretty stable.

Look at the files changed, not the commit list - it's extensive. I suggest merging it as a squash commit.

[gma]

Cheers for the pull request.

I'm afraid this is going to have to stay parked for quite a while; I've had a look at the patch but I'd need to spend a lot longer with it than I have just now to be confident about how I'd like to proceed with it. I'm bootstrapping a business at the moment and need to focus on getting that to a stage where it's making revenue, so I don't have time for big jobs on Nesta at the moment. I just wanted to let you know that action won't be imminent just yet – I'll take a proper look at it in the future when I've got more time to dedicate to Nesta.

[gma]

See #74 for earlier discussion.

[ms]

I'm looking at it currently and it looks pretty clean. I'll test it more extensively and report anything I find.

Are there any test of the new functionality? (Like registering a new renderer, stuff like that)

[lilith]

I haven't written any tests for it, unfortunately; I don't know rspec that well.

[ms]

I just ran the specs and there are 3 errors:

1)
'Markdown page with assigned pages should not find pages scheduled in the future' FAILED
expected nil, got #<Nesta::Page:0x10e210340 @metadata={"date"=>"16 August 2012"}, @date=#<DateTime: 4912311/2,0,2299161>, @markup="#  Article 4\n\nContent goes here\n", @filename="/Users/max/Code/Github/nesta/spec/tmp/content/pages/foo/article-4.mdown", @mtime=Wed Aug 15 18:16:27 -0700 2012, @format=:mdown>
/Users/max/Code/Github/nesta/spec/models_spec.rb:197:
/Users/max/.rvm/gems/ruby-1.8.7-p370/bin/ruby_noexec_wrapper:14:

2)
'Haml page with assigned pages should not find pages scheduled in the future' FAILED
expected nil, got #<Nesta::Page:0x10e556ce8 @metadata={"date"=>"16 August 2012"}, @date=#<DateTime: 4912311/2,0,2299161>, @markup="%div\n  %h1 Article 4\n\nContent goes here\n", @filename="/Users/max/Code/Github/nesta/spec/tmp/content/pages/foo/article-4.haml", @mtime=Wed Aug 15 18:16:27 -0700 2012, @format=:haml>
/Users/max/Code/Github/nesta/spec/models_spec.rb:197:
/Users/max/.rvm/gems/ruby-1.8.7-p370/bin/ruby_noexec_wrapper:14:

3)
'Textile page with assigned pages should not find pages scheduled in the future' FAILED
expected nil, got #<Nesta::Page:0x10e20ce70 @metadata={"date"=>"16 August 2012"}, @date=#<DateTime: 4912311/2,0,2299161>, @markup="<div>\nh1. Article 4\n\nContent goes here\n", @filename="/Users/max/Code/Github/nesta/spec/tmp/content/pages/foo/article-4.textile", @mtime=Wed Aug 15 18:16:28 -0700 2012, @format=:textile>
/Users/max/Code/Github/nesta/spec/models_spec.rb:197:
/Users/max/.rvm/gems/ruby-1.8.7-p370/bin/ruby_noexec_wrapper:14:

I didn't look into it but it's pretty obvious that somehow the check of the time of the page broke.

@nathanaeljones About writing more tests for the new feature, I'll look into it but it's really not too difficult. You can look at the existing ones and or at tutorial but the basics are describe "Your feature" do it "should do something" do should_*([something]) end end.

Another concern I have is how cleanly your modifications merge with @gma's more recent additions. What I can tell you is that git merge gma/master gives:

Removing lib/nesta/nesta.rb
Auto-merging lib/nesta/navigation.rb
CONFLICT (content): Merge conflict in lib/nesta/navigation.rb
Auto-merging lib/nesta/models.rb
CONFLICT (content): Merge conflict in lib/nesta/models.rb
Auto-merging lib/nesta/app.rb
CONFLICT (content): Merge conflict in lib/nesta/app.rb
CONFLICT (modify/delete): Gemfile.lock deleted in HEAD and modified in gma/master. Version gma/master of Gemfile.lock left in tree.
Automatic merge failed; fix conflicts and then commit the result.

Doesn't look too good. So if you could merge @gma's master into your repo and fix the conflicts (it'll add it here automatically) it'd be great, to allow more complete testing and meaningful specs.

Cheers.

[ms]

Update on the "article in the future" thing. The method is in models.rb:196 (before you try to merge gma/master, that'll add lines of conflicting code). All I can say at this point is that it is getting called.

[gma]

Thanks Max, I really appreciate you taking the time to investigate.

I wouldn't recommend either of you spending a significant amount of time on this for the moment though. I'm behind the feature, but until I've been through the implementation/approach more carefully I wouldn't be too sure whether or not this patch will become the way it's done or not. It may, but the point is I'm not sure about that so I'd rather you didn't spend time working on it without being aware that I might want to approach it slightly differently.

The lack of tests is a good point. I'd rather it was TDD'd, rather than have tests added after the fact. I know I'm probably more precious than I need to be about that, but there's no doubt that it makes a difference, and the benefits are well worth it.

On 16 Aug 2012, at 02:32, Max Sadrieh notifications@github.com wrote:

Update on the "article in the future" thing. The method is in models.rb:196 (before you try to merge gma/master, that'll add lines of conflicting code). All I can say at this point is that it is getting called.

— Reply to this email directly or view it on GitHub.

[gma]

Closing this so that the open issues is closer to an actionable todo list for 0.10.0 and 1.0.0. We can re-open later if appropriate.