jish / pre-commit

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

With RVM use _current_ ruby and gemset, not _default_ #201

Closed eikes closed 9 years ago

eikes commented 9 years ago

I was getting errors, when trying to use rubocop in pre-commit, because it couldn't be found. But I had installed it in the current gemset. The problem was, that pre-commit was not using the current gemset, but the default. This commit fixes that.

mpapis commented 9 years ago

the default was used on purpose, if you prefer to use current take advantage of https://github.com/eikes/pre-commit/blob/patch-1/templates/hooks/automatic#L12 and set

git config pre-commit.ruby "rvm `rvm current` do ruby"

or

git config pre-commit.ruby "rvm current do ruby"

closing as this would change the planned / expected behavior and you have options to change this with git config

eikes commented 9 years ago

Please explain, why this is the intended behavior.

The whole point of rvm and gemsets is, that when you are in a certain directory you will use the ruby version and gemset that is configured. By using the default ruby and gemset you are breaking this:

If I add "pre-commit" to my Gemfile and run bundler, I will not be able to use it:

pre-commit: WARNING: Skipping checks because: cannot load such file -- pre-commit

I would need to go to a directory without a rvm configuration and 'gem install pre-commit' there (and rubocop), so it appears in my default gemset. That doesn't make a lot of sense.

coveralls commented 9 years ago

Coverage Status

Coverage remained the same at 98.65% when pulling 9177584fbcdd6bc45ba7442b09809c7cae644bf0 on eikes:patch-1 into 5a1eb5e14593b43da1cd2bfcbefff6e66d96a42e on jish:master.

mpapis commented 9 years ago

we(pre-commit&rvm) assumed that when you use pre-commit you want to use it on more then one project, for that default works best, if you want to have pre-commit per project just use the git config.

to install pre-commit in default ruby you can use:

rvm default do gem install pre-commit
eikes commented 9 years ago

This assumption is in direct conflict with the RVM approach, which is that I can take complete control of the ruby environment that I am currently using. Having to use the same version of pre-commit, execjs and rubocop (and dependencies) in all projects is not what RVM is made for.

Why don't you invert that assumption and say, that if someone wants to use a global (default) version, he can do so by configuring it:

git config pre-commit.ruby "rvm default do ruby"

But make pre-commit use the current gemset by default?

mpapis commented 9 years ago

from RVM side - the problem with current is - that pre-commit is ran with a new sh shell - and you might get something else then your shells environment, @jish any problems on pre-commit side regarding default vs. current?

jish commented 9 years ago

Unfortunately this is the number one issue with this gem. Everyone has different environment setups and everyone wants it to work out of the box their way, in their environment.

We obviously want to make this work properly for as many people as possible. Unfortunately, I have not collected any stats on which environments everyone uses over the years.

As @mpapis pointed out, we try to provide configuration hooks to make everyone happy. There are options to configure your preferred Ruby using git configuration. There are also multiple hook templates pre-commit install --automatic, pre-commit install --manual. We could create a new one: pre-commit install --rvm-current.

Again, I'm on board with using settings that make the most people happy, and if that means using the current gemset, I'm all for it (but that also assumes most people are using RVM, and are using gemsets). Of course, when / if a breaking change like happens, it would require a major version bump.

mpapis commented 9 years ago

well you do not need to stick to semantic versioning before you reach 1.0, the problem is that the feedback loop takes a long time for the change to be recognized, many people would just use the defaults for years because it works and you would not get any feedback until they need to install the gem on new machine.

eikes commented 9 years ago

Adding another template sounds reasonable. The very least that should happen is to add documentation about the fact that the standard installation does not use the current gemset.

I could write the --rvm-current template and necessary code and open another PR.

mpapis commented 9 years ago

we can achieve the same result as with --rvm-current using only documentation, which would be also needed for the current template - do we really need add code for a problem that is solvable on documentation level? - more code - bigger maintenance cost (time).