search

Scrimmage / karma-slim-preprocessor

Compiles slim to html for karma tests
MIT License
5 stars 6 forks source link

added option to provide slimrb binary location #2

Closed arkarkark closed 10 years ago

arkarkark commented 10 years ago

fixes https://github.com/Scrimmage/karma-slim-preprocessor/issues/1

arkarkark commented 10 years ago

Hi, I'm wondering what you thought of this PR? I kinda need it for my project and I'd rather not fork your slim-preprocessor if this'll get accepted soon.

arkarkark commented 10 years ago

forked into npm package: karma-slim-preprocessor-with-binary-option

ratbeard commented 10 years ago

Hey @arkarkark, thanks for making this PR! I'm sorry I didn't see it sooner, I must have disabled my github notifications :crying_cat_face: I was so excited to see the first issue, I fixed it without seeing your PR first before fixing it in master :crying_cat_face: :crying_cat_face:

My fix looked pretty identical to yours, I cleaned up the jshint stuff too. I liked your property name 'slimrb' better than 'command' so I just changed mine to that. I did some other stuff like improve the error handling and added a few more tests. I borrowed your travis tweaks as well though one test I'm not able to get running on travis yet though it works locally.

Looking at how I'm handling the 'slimrb command not found', I think I'm going to do one more round of refactoring and try and detect that once when the preprocessor is instantiated instead of when processing every file as I think that is whats causing the travis test to fail and detecting slim syntax vs slimrb not found is kinda janky atm. When I push that up would you mind giving it a try before I bump the npm version? You should be able to install it with npm install Scrimmage/karma-slim-preprocessor --save-dev straight from git(hub).

ratbeard commented 10 years ago

Pushed up a cleaned up fix, closing PR and moving discussion to #1