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

Initial implementation of directives; include implemented. #55

Merged
merged 2 commits into from
Feb 15, 2013
Merged

Initial implementation of directives; include implemented. #55

merged 2 commits into from
Feb 15, 2013

Conversation

mehcode
Copy link
Collaborator

@mehcode mehcode commented Feb 8, 2013

I've started work on the directives. I've got the first bit done (having the compiler recognize directives) and implemented include. I figure it's better to do this in pieces as #50 is a bit large.

The implementation works nicely (at least in my testing) for placement in global as well as AMD (it could work for standalone too but I can add that after that PR is merged I suppose).

I attempted to follow your existing conventions around the code base when adding directives; let me know where the implementation could be improved, etc.


Also... I couldn't figure out how to get jasmine to unit test the include directive as two templates need to be compiled and one rendered and your testing harness only allows for one and one. Should I add more specs or do you have another idea?

# Extend the options with the name and namespace so that the
# compiler has these configuration properties from the beginning
# and that the API for this method can stay the same.
options.namespace = namespace
Copy link
Collaborator

Choose a reason for hiding this comment

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

What about deprecating these params and just ask for (source, options = {}) ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'd prefer that; thanks for mentioning it. I meant to mention it in the post. It's something I was going to ask. @netzpirat what do you think?

Copy link
Owner

Choose a reason for hiding this comment

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

I'm fine with the change, but this function is part of the public API and we need to make sure that it's done in a backward compatible way, so that both function signatures are supported.

@netzpirat
Copy link
Owner

Thanks a lot for the initial implementation. I just returned from holidays and will try it within the next few days and have also a look on possible testing strategies for cases where multiple templates are involved.

@netzpirat
Copy link
Owner

FYI, I didn't forget this, I'm just feeling sick and I'm not able to dig into this right now.

@mehcode
Copy link
Collaborator Author

mehcode commented Feb 12, 2013

@netzpirat Take your time. Hope you feel better soon.

@netzpirat netzpirat merged commit 816e303 into netzpirat:master Feb 15, 2013
@netzpirat
Copy link
Owner

Just extended the spec suite to setup globally placed partials, which makes integration tests for the +include directive possible. I didn't do it for the amd placement, mainly because I'm short on time and don't want to spend to much time on stuff I don't need personally. I've added you as contributor, so you can directly push to master if you want to contribute more time and features to Haml-Coffee. Thanks a lot!

@vendethiel
Copy link
Collaborator

🍰

@netzpirat
Copy link
Owner

@Nami-Doc I added you also as contributor. I try to follow an open commit bit policy on my repositories like @jfirebaugh

@vendethiel
Copy link
Collaborator

not sure why but thanks I suppose :p

@netzpirat
Copy link
Owner

@Nami-Doc Because you've made a pull request that was accepted.

@mehcode mehcode deleted the topics/include branch February 15, 2013 22:39
@vendethiel
Copy link
Collaborator

@netzpirat
Copy link
Owner

@Nami-Doc Great read, thanks for sharing.

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