search

jish / pre-commit

A slightly improved pre-commit hook for git
https://jish.github.io/pre-commit/
Other
796 stars 96 forks source link

Add execjs to dependencies #269

Closed shuheiktgw closed 6 years ago

shuheiktgw commented 6 years ago

This PR relates to https://github.com/jish/pre-commit/issues/234

When I run pre-commit, it warns me Could not load execjs: cannot load such file -- execjs. I found In js plugin, pre-commit checks if it can load execjs or not. I think it is kind of inconvenient I need to manually install execjs in order to use js plugin, so added it to dependency! 👍

jish commented 6 years ago

Hmm... let me think about this. I'm not sure if the execjs dependency is missing on purpose or by accident. 🤔

On the one hand we want to ship a working program out of the box, so execjs should be an explicit dependency.

But on the other hand, I believe there are some projects that want to put pre-commit in their Gemfile and also do not want execjs as part of their bundle.

@shajith do you have an opinion on this?

shuheiktgw commented 6 years ago

Oh, now I see the intention of this code! 👍 Hmm, what if I discard this change, and just make the warning message more developer friendly to tell the developers that they need to add execjs to their dependency manually?

jish commented 6 years ago

"Oh, now I see the intention of this code! 👍" "Hmm, what if I discard this change, and just make the warning message more developer friendly to tell the developers that they need to add execjs to their dependency manually?"

👍 That would be great. A better message would be helpful no matter what path we choose going forward 🙂

shuheiktgw commented 6 years ago

Thank you for your comment! I just made another PR https://github.com/jish/pre-commit/pull/273!