Replace regex with Bundler::LockfileParser

[amomchilov]

Depends on #12186, see the difference: amomchilov/rubocop@add-requires_gem-api...parse-lockfile-with-bundler

Now that we have a Hash of all of the target's gems, we can just use that target_rails_version_from_bundler_lock_file, and remove the manual regex parsing.

---

Before submitting the PR make sure the following are checked:

• The PR relates to only one subject with a clear title and description in grammatically correct, complete sentences.
• Wrote good commit messages.
• Commit message starts with [Fix #issue-number] (if the related issue exists).
• Feature branch is up-to-date with master (if not - rebase it).
• Squashed related commits together.
• Added tests.
• Ran bundle exec rake default. It executes all tests and runs RuboCop on its own code.
• Added an entry (file) to the changelog folder named {change_type}_{change_description}.md if the new code introduces user-observable changes. See changelog entry format for details.

[koic]

Is the Bundler::LockfileParser a stable published API that can be used safely? I'm concerned about whether it's a private API intended for internal use within Bundler, which might introduce breaking changes in future Bundler versions.

[amomchilov]

I removed this memoization because:

1. If read_rails_version_from_bundler_lock_file returned nil, it wouldn't memoize correctly and would be trying to parse the lockfile on every call anyway
2. This value is only accessed from #target_rails_version, which is itself memoized

[amomchilov]

Hey @koic!

I don't see any firm evidence that it's intended to be public (e.g. YARD docs), but I think it's very likely to be the case.

• It's mentioned once in the changelog (which would be unusual if it was intended to be only an internal implementation detail)
• There are a lot of usages on GitHub, including some pretty prominant gems:
  • Ruby/ruby: https://github.com/ruby/ruby/blob/3602e253e79b796661168b458e22ba1694ec06e5/lib/bundler/self_manager.rb#L171
  • Rails/spring: Use Bundler::LockfileParser to parse lockfile instead of regex rails/spring#492

Package management is central to an ecosystem, and being able to introspect dependencies is pretty core to a bunch of tooling. If it's not public, we should perhaps try to make it public, like how Gem::Version is used throughout the ecosystem today.

Being dependant on Bundler::LockfileParser isn't well justified by this PR on its own, but I needed it for #12186. Once that's implemented, we can use it here for "free".

[koic]

@deivid-rodriguez @hsbt May I ask the Bundler development team, Is Bundler::LockfileParser intended to be used as a published API? I'm concerned about potential incompatibilities arising from the use of private API.

[deivid-rodriguez]

Hei @koic. Bundler::LockfileParser is not documented anywhere as a public API, but I've seen many cases of people using it. So We don't plan to intentionally break things for people. That said, given it's not documented at the moment, it should still be "used at your own risk". We could probably formalize an API at some point.

[koic]

@deivid-rodriguez I've come to understand the Bundler::LockfileParser API. Thank you very much!

[deivid-rodriguez]

No problem!

[bbatsov]

I don't mind using Bundler::LockfileParser. @amomchilov Let us know when you feel the PR is ready for review.

[amomchilov]

@bbatsov добър вечер!

I've just rebased it. This PR builds on top of #12186, which needs to be reviewed and shipped first. After that, I think this PR is also ready for review/merge (just look at the last commit)

[koic]

The commits related to the changes in #12180 are mixed in. Can you leave the change for only "Replace regex with Bundler::LockfileParser"?

[koic]

It just occurred to me that RuboCop doesn't have a runtime dependency on Bundler. This means to utilize Bundler's API, a runtime dependency on Bundler would be required. However, introducing such a dependency solely for the sake of using Bundler::LockfileParser seems like an overkill.

Perhaps continuing to resolve this with regexp would be a more appropriate approach. In other words, it might be better if this change were not made.

[amomchilov]

The commits related to the changes in #12180 are mixed in.

Hey @koic, did you mean #12186? This "Replace regex with Bundler::LockfileParser" change depends on the code introduced in the other PR. The other PR would need to be merged first. Could you please take a look?

However, introducing such a dependency solely for the sake of using Bundler::LockfileParser seems like an overkill.

Perhaps continuing to resolve this with regexp would be a more appropriate approach. In other words, it might be better if this change were not made.

@koic For this change on its own, I would agree. But the real benefit here is new requires_gem feature I've proposed and implemented in #12186. Once that PR introduces the ability to parse the lockfile, this PR just becomes a natural next step: to replace this manual lockfile parsing with the results we've already got from #12186.

[bbatsov]

I agree it's not prudent to add a runtime dependency on Bundler, so I see three paths ahead:

• Using this parser only when available
• Inlining something similar into RuboCop (I'm not sure how complex its implementation is)
• Keeping things as they are

[amomchilov]

@bbatsov You raise an interesting point, I'll think on it!

In the mean time, could I get your thoughts on this feature request and the implementation I proposed for it?

Is the requires_gem API useful enough to justify the bundler dependency? Or perhaps I should extract it into a module that you mix into your cops as-needed, and then make the bundler dependency only required for that.

[amomchilov]

Just ran into this, apparently this is already used by RuboCop

rubocop/lib/rubocop/lockfile.rb

Lines 30 to 38 in a455e9d

	def parser
	return unless defined?(Bundler) && Bundler.default_lockfile
	return @parser if defined?(@parser)
	lockfile = Bundler.read_file(Bundler.default_lockfile)
	@parser = lockfile ? Bundler::LockfileParser.new(lockfile) : nil
	rescue Bundler::BundlerError
	nil
	end

[bbatsov]

Ah, excellent - so I guess you know how to proceed now. :-)

[bbatsov]

@amomchilov Any updates here?

[amomchilov]

Hey @bbatsov I had a question about how to proceed. Imessaged you in the RuboCop discord (I figured that was a good spot, because it showed you as online). Cross posting:

---

Добър вечер @bbatsov !

I'm taking a look at using the RuboCop::Lockfile abstraction that already exists in the codebase. It has its own logic to find the relevant lockfile for the repo (Bundler.default_lockfile) rather than the custom logic that exists in RuboCop::Config#bundler_lock_file_path.

They both do some directory traversal to find a gemfile in a parent dir, if it's not found in the cwd. Not sure if they're exactly the same, though 🤔

I'm not sure how to proceed. Do you think I should modify RuboCop::Lockfile to take in the path as a param (as determined by the existing RuboCopConfig#bundler_lock_file_path logic), or should I just use whatever RuboCop::Lockfile does already?

[bbatsov]

I'm not sure how to proceed. Do you think I should modify RuboCop::Lockfile to take in the path as a param (as determined by the existing RuboCopConfig#bundler_lock_file_path logic), or should I just use whatever RuboCop::Lockfile does already?

I'd be fine with both approaches, but the first one sounds slightly better to me.

[amomchilov]

@bbatsov Can we move the lock-file parsing related discussion to the underlying PR? That's what introduces that logic, it's an impl detail separate from the changes in this PR.

I responded here: #12186 (review)

[bbatsov]

I guess you can rebase this one now that #12186 is finally merged.

[amomchilov]

@bbatsov Rebased, and ready to merge!

Thanks for jumping in to merge all these pieces :)