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

added template inheritance #186

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

added template inheritance #186

wants to merge 8 commits into from

Conversation

csingewald
Copy link

I added template inheritance as proposed in mustache/spec#38. There where problems using the '$' sign in the parser thats why I chose the '%'. The '$' sign would be a better choice.

@locks
Copy link
Member

locks commented Jan 19, 2015

Thank you! I'll review it in the weekend and get back to you.

@@ -1,3 +1,3 @@
class Mustache
VERSION = '1.0.0'
VERSION = '1.0.1'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please don't change the version number.
I'll bump it previous to releases, it makes it easier to track.

@locks
Copy link
Member

locks commented Jan 26, 2015

You can ignore the comments apart from the version number one, that one I would appreciate if you could make the change.

@bobthecow
Copy link
Member

Using % as the block sigil might be a deal-breaker, since a large part of the reason people choose mustache is that it works in multiple environments. Template inheritance has already been implemented in Java, JavaScript, Objective-C and PHP (at least) using the $ sigil.

What sort of problems were you running into? Was it a regular expression issue?

@locks
Copy link
Member

locks commented Jan 26, 2015

Thanks @bobthecow, I actually meant to ask that but seems I forgot.

@csingewald
Copy link
Author

Thank you for the review, I am still learning a lot. I startet with ruby only a few weeks ago. I pushed the changes.
Using the $ sign parsing the template failed sometimes. It seemed not to be deterministic. Inside the parser method 'scan_tag_close' the prematch was sometimes nil. I could not figure out why.

@locks
Copy link
Member

locks commented Feb 9, 2015

Hi @singy.

Like @bobthecow mentioned, it's a problem if this implementation uses a different sigil (% instead of $) from the other implementations for template inheritance.

I tried but couldn't figure out what exactly is making the tests not pass if we use $. If you have the time I'd appreciate your help so we can finish this :)

@bobthecow
Copy link
Member

Also, {{%PRAGMA}} is used for pragmas, and while the ruby implementation doesn't currently have any pragmas, it has in the past, and probably should have one for this change, since it isn't strictly spec compliant :)

@ghost
Copy link

ghost commented Feb 10, 2015

What is the problem with using $, is it anything to do with the way it compiles to a Ruby string? What's failing?

@csingewald
Copy link
Author

I agree with you, the '$' sign ist the way to go. I changed the source. One test fails, maybe I am wrong and this test is erroneous.

https://github.com/singy/mustache/commit/f81e9b1b7c29cadb3c64522785c5de5d3bc8e96e

@locks
Copy link
Member

locks commented Feb 10, 2015

@singy well this is confusing. Travis reports 3 errors, I'm getting 4 locally, and you get only 1 xD
What Ruby version are you running?

mine: ruby-2.1.2
travis: 2.1.0p0 https://travis-ci.org/mustache/mustache/jobs/50105158#L126

@locks
Copy link
Member

locks commented Feb 10, 2015

Interesting, I get two errors if I run rake test instead of bundle exec rake test.

It's order-dependent, varying between 1 and 4 errors. This exposes a problem somewhere in the codebase.

@@ -298,6 +298,15 @@ def scan_tag_block content, fetch, padding, pre_match_position
end
alias_method :'scan_tag_#', :scan_tag_block

def scan_tag_blockvar content, fetch, padding, pre_match_position
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

end

# load and process parent template
ev(<<-compiled)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@homu
Copy link
Collaborator

homu commented May 12, 2016

☔ The latest upstream changes (presumably #220) made this pull request unmergeable. Please resolve the merge conflicts.

@ghost
Copy link

ghost commented May 12, 2016

For anyone wanting template inheritance/layouts until this patch is accepted, I have implemented it I'm my Ruby Mustache implementation called Tache and am using it in production.

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

Successfully merging this pull request may close these issues.

4 participants