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

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

Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

eikes
Copy link
Contributor

@eikes eikes commented Feb 20, 2015

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.

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
Copy link
Collaborator

mpapis commented Feb 20, 2015

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

@mpapis mpapis closed this Feb 20, 2015
@eikes
Copy link
Contributor Author

eikes commented Feb 20, 2015

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
Copy link

Coverage Status

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

@mpapis
Copy link
Collaborator

mpapis commented Feb 20, 2015

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
Copy link
Contributor Author

eikes commented Feb 20, 2015

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
Copy link
Collaborator

mpapis commented Feb 20, 2015

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
Copy link
Owner

jish commented Feb 23, 2015

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
Copy link
Collaborator

mpapis commented Feb 23, 2015

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
Copy link
Contributor Author

eikes commented Feb 24, 2015

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
Copy link
Collaborator

mpapis commented Feb 24, 2015

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).

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.

4 participants