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
[Fix #9580] Add New Cop to Enforce Bundler Gem filename #9903
[Fix #9580] Add New Cop to Enforce Bundler Gem filename #9903
Conversation
3bca3cb
to
a3db688
Compare
# This cop verifies that a project contains Gemfile or gems.rb file and correct | ||
# associated lock file based on the configuration. | ||
# | ||
# @example RequiresGemfile: true (default) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd suggest "EnforcedStyle" with the options "Gemfile" and "gems.rb". To me at least, that's much easier to understand.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Absolutely agree, and it's more consistent with other cops that way.
class GemFilename < Base | ||
include RangeHelp | ||
|
||
MSG_GEMFILE_REQUIRED = 'gems.rb file was found but Gemfile is required.' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add backticks around the names of the files, so they are properly colored in the terminal.
a3db688
to
b308532
Compare
file_path = processed_source.file_path | ||
return if expected_gemfile?(file_path) | ||
|
||
register_offense(processed_source, file_path) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see this function uses only the underlying buffer, so I guess you can pass it down directly.
MSG_GEMS_RB_MISMATCHED | ||
end | ||
|
||
return if message.nil? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's a bit weird that this function might register an offense or not. Perhaps it has to be separated in two for clarity.
b308532
to
ad5892d
Compare
MSG_GEMFILE_MISMATCHED | ||
end | ||
|
||
add_offense(source_range(source_buffer, 1, 0), message: message) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe you should be able to use add_global_offense
instead of the two add_offense
calls since the offense isn't really related to a line.
rubocop/lib/rubocop/cop/base.rb
Lines 105 to 112 in 0741555
# Adds an offense that has no particular location. | |
# No correction can be applied to global offenses | |
def add_global_offense(message = nil, severity: nil) | |
severity = find_severity(nil, severity) | |
message = find_message(nil, message) | |
@current_offenses << | |
Offense.new(severity, Offense::NO_LOCATION, message, name, :unsupported) | |
end |
def gemfile_offense?(file_path) | ||
gemfile_required? && %w[gems.rb gems.locked].include?(file_path) | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
def gemfile_offense?(file_path) | |
gemfile_required? && %w[gems.rb gems.locked].include?(file_path) | |
end | |
def gemfile_offense?(file_path) | |
gemfile_required? && GEMS_RB_FILES.include?(file_path) | |
end |
def gems_rb_offense?(file_path) | ||
gems_rb_required? && %w[Gemfile Gemfile.lock].include?(file_path) | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
def gems_rb_offense?(file_path) | |
gems_rb_required? && %w[Gemfile Gemfile.lock].include?(file_path) | |
end | |
def gems_rb_offense?(file_path) | |
gems_rb_required? && GEMFILE_FILES.include?(file_path) | |
end |
The other bundler cops handle gem/lock files that aren't in the root directory, but this doesn't appear like it'll do so given how the file names are matched. |
@gregfletch Ping :-) |
Oops, completely forgot about this. Should have time by this weekend to fix up the remaining comments and finish this off. Sorry about that! |
Add a new cop which enforces which bundler gem filename to use. By default, enforces Gemfile and its related Gemfile.lock file. Alternatively, setting RequiresGemfile to false enforces gems.rb and its related gems.locked file.
…ply to non-root gem files
ad5892d
to
591b96b
Compare
Seems we're good to go now. Thanks! |
Add a new cop which enforces which bundler gem filename to use. By default, enforces Gemfile and its related Gemfile.lock file. Alternatively, setting RequiresGemfile to false enforces gems.rb and its related gems.locked file.
Before submitting the PR make sure the following are checked:
[Fix #issue-number]
(if the related issue exists).master
(if not - rebase it).bundle exec rake default
. It executes all tests and runs RuboCop on its own code.{change_type}_{change_description}.md
if the new code introduces user-observable changes. See changelog entry format for details.