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

PatternEngines: support multiple template engines #173

Closed
wants to merge 18 commits into from

Conversation

geoffp
Copy link
Contributor

@geoffp geoffp commented Oct 27, 2015

EDIT: newer PR pointed at feature branch is here:
#191

NOT READY FOR MERGE YET, this PR is only for review.

@bmuenzenmeyer, I've made some progress on a PatternEngine architecture. It's not remotely done or cleaned up yet, so please excuse any whitespace fussing, but I was hoping you'd look at it to see where I'm going with it and give me feedback. My approach is to incrementally move mustache-specific functionality into a new mustache PatternEngine as it becomes available. It's currently passing all unit tests (EDIT: no it's not, I just found npm test; it does pass grunt nodeunit), though lots more direct uses of mustache still need to be moved into the PatternEngine.

It's also a goal to be able to support (and unit test) whatever PatternEngines are implemented, but only activate them if their backing engines are installed by the end user as npm deps. So, if you npm install handlebars, the Handlebars PatternEngine and its unit tests should wake up and start doing things.

@geoffp
Copy link
Contributor Author

geoffp commented Oct 27, 2015

I think I see what's broken; I'll fix it hopefully tomorrow.

@bmuenzenmeyer
Copy link
Member

wow @geoffp this is a great contribution and time investment - THANK YOU! 😲

I will need to take some time to read through all this - hopefully this weekend. appreciate the work and the in process sharing. I wonder if it would help to push this to a feature branch rather than dev for now ?

@geoffp
Copy link
Contributor Author

geoffp commented Oct 28, 2015

Yeah, I think a feature branch would be good. I'm sorry for the large code drop, I know it'll be hard to review. That's why I wanted to get this work in front of your eyes sooner rather than later.

@bmuenzenmeyer
Copy link
Member

Hi @geoffp I just finished the work I had wanted to wrap up with Style Modifiers. Can you see if you can pull those changes and then point this PR to a feature branch? In the meantime I will start reviewing.

Thanks

@geoffp
Copy link
Contributor Author

geoffp commented Nov 6, 2015

Excellent! Sure thing. I'll merge the latest as soon as I can and then see if I can pass the unit tests as well.

Do you need to create the feature branch in this repository, or can I?

@bmuenzenmeyer
Copy link
Member

Dunno if you can. I created https://github.com/pattern-lab/patternlab-node/tree/pattern-engines for you to hit

@bmuenzenmeyer
Copy link
Member

@geoffp ping. just making sure you saw the above.

@geoffp
Copy link
Contributor Author

geoffp commented Nov 9, 2015

Yes, I did. I'm trying to get the time to do this ASAP. Stay tuned! 😀
On Mon, Nov 9, 2015 at 3:49 PM Brian Muenzenmeyer [email protected]
wrote:

@geoffp https://github.com/geoffp ping. just making sure you saw the
above.


Reply to this email directly or view it on GitHub
#173 (comment)
.

@bmuenzenmeyer
Copy link
Member

No rush. Believe me, I understand how time flies 🕥

…pattern-engines

Conflicts:
	builder/object_factory.js
	builder/parameter_hunter.js
	builder/pattern_assembler.js
	builder/patternlab.js
don't render at all, and are missing in the menu. The quest continues...
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.

3 participants