search

gulp-community / gulp-haml

gulp plugin for Haml
MIT License
25 stars 20 forks source link

Added support for visionmedia/haml.js (faster compiler) and updated README and tests accordingly. #7

Closed txbm closed 10 years ago

txbm commented 10 years ago

All tests pass, docs are up to date. Would love to get this up on NPM if it's acceptable.

stephenlacy commented 10 years ago

Adding that option would be beneficial, though there are a few style issues if you do not mind solving before I can merge:

I do not use that code style, and I don't wrap main functions (index and tests) as anonymous. Style guide used: https://github.com/felixge/node-style-guide Use lodash.clone instead of doing the mergeObjects Don't use underscores before function names inside a function scope that are not cloned. Make sure the objects follow: https://github.com/felixge/node-style-guide#object--array-creation as well

You could also switch map-stream to through2 to help with error reporting as well if you want to.

Thanks

txbm commented 10 years ago

I've made the style updates. I think you meant lodash.merge.

I don't need the error reporting cleanup at the moment but I'm sure I'll get around to it at some point.

stephenlacy commented 10 years ago

Thanks for the style changes! One last detail, could you lay the objects with two space indents instead of four and have the trailing brace in line with the opening var? https://github.com/petermelias/gulp-haml/blob/4d5dde611453f82b098d6303706856b2fd5125ea/index.js#L8

It is part of the node-style guide. Thanks

txbm commented 10 years ago

done.

stephenlacy commented 10 years ago

Thank you, could you also do the same on the main test file?

txbm commented 10 years ago

done

stephenlacy commented 10 years ago

Excellent!

Thank you, merging.