[#5473] Infer TargetRailsVersion from bundler lock files #5518
Conversation
|
@pocke I took a stab at the issue. You mentioned using bundler to read the lock file, but I wasn't sure if we wanted to add a dependency on bundler at that point in the code. I stuck with the regex matching approach for the first cut, but I'm happy to change it. |
spec/rubocop/config_spec.rb
Outdated
| before do | ||
| allow(configuration).to( | ||
| receive(:bundler_lock_file_name).and_return(lock_file_path) | ||
| ) |
There was a problem hiding this comment.
Probably you can use isolated_environment instead of the mock.
https://github.com/bbatsov/rubocop/blob/698bd2ee06aa365d85850a2ff23f71c46e0fab06/lib/rubocop/rspec/shared_contexts.rb#L6
it creates a temporary directory for the test.
I think that we should avoid mock as much as possible.
spec/rubocop/config_spec.rb
Outdated
| after { FileUtils.rm_f(lock_file_path) } | ||
|
|
||
| it 'uses the Rails version when Rails is present in the lock file' do | ||
| create_file(lock_file_path, "Foo\n rails (4.1.0)\n bar\n\n") |
There was a problem hiding this comment.
I prefer using a more realistic lock file. For example:
GEM
remote: https://rubygems.org/
specs:
actionmailer (4.1.0)
actionpack (= 4.1.0)
actionview (= 4.1.0)
mail (~> 2.5.4)
rails (4.1.0)
actionmailer (= 4.1.0)
actionpack (= 4.1.0)
actionview (= 4.1.0)
activemodel (= 4.1.0)
activerecord (= 4.1.0)
activesupport (= 4.1.0)
bundler (>= 1.3.0, < 2.0)
railties (= 4.1.0)
sprockets-rails (~> 2.0)
PLATFORMS
ruby
DEPENDENCIES
rails (= 4.1.0)
BUNDLED WITH
1.16.1
spec/rubocop/config_spec.rb
Outdated
| create_file(lock_file_path, "Foo\nbar\n\n") | ||
| default = RuboCop::Config::DEFAULT_RAILS_VERSION | ||
| expect(configuration.target_rails_version).to eq default | ||
| end |
There was a problem hiding this comment.
We also need a test case for "when Gemfile.lock does not exist" case.
|
Thanks for the feedback. I'll make the changes later today. |
|
I think I've made the necessary adjusts. It's ready for a second look. |
| end | ||
|
|
||
| def read_rails_version_from_bundler_lock_file | ||
| lock_file_path = bundler_lock_file_path |
There was a problem hiding this comment.
AFAIK, If this cop search strictly for lockfile, it need to use the following Bundler's method.
Bundler.default_lockfile
Because the mechanism to search the lockfile is complicated as follows.
https://github.com/bundler/bundler/blob/v1.16.1/lib/bundler/shared_helpers.rb#L240-L246
Of course, now RuboCop doesn't depend on Bundler, so we can not use Bundler.default_lockfile. Please refer to when considering @pocke's advice 😃
#5473 (comment)
There was a problem hiding this comment.
Not a big concern right now.
|
It has to be rebased on top of the current master, but it looks OK to me overall. My only slight concern is that users might not ever realize such an inference is happening, which might not be a real problem in practice. |
|
Thanks for taking a look. I rebased the branch. |
|
Thanks! |
Closes #5473.
Change the config init process slightly for
TargetRailsVersionto look ingems.lockedorGemfile.lockfor the best value. Either of those files will lock Rails to a specific version and we can use that information to dynamically pick the best value forTargetRailsVersion.If
TargetRailsVersionis set in the config file, it still takes precedence and is always used. If Rails is not ingems.lockedorGemfile.lock, and the user hasn't specified a version in the config, the default version is still used as the fallback.Before submitting the PR make sure the following are checked:
[Fix #issue-number](if the related issue exists).master(if not - rebase it).and description in grammatically correct, complete sentences.
rake defaultorrake parallel. It executes all tests and RuboCop for itself, and generates the documentation.