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

[#3600] Add new `Bundler/DuplicatedGem` cop #3638

Merged
merged 1 commit into from Nov 1, 2016

Conversation

Projects
None yet
7 participants
@jmks
Contributor

jmks commented Oct 17, 2016

A part of #3600

This cop checks that gems are only listed once in a Gemfile.
I think bundler prints a warning if there's a duplicate, and only raises an error if there's a dependency issue (e.g. different versions). I don't know of a case when it's necessary to list a gem twice.


Before submitting the PR make sure the following are checked:

  • Wrote good commit messages.
  • Commit message starts with [Fix #issue-number] (if the related issue exists).
  • Used the same coding conventions as the rest of the project.
  • Feature branch is up-to-date with master (if not - rebase it).
  • Squashed related commits together.
  • Added tests.
  • Added an entry to the Changelog if the new code introduces user-observable changes. See changelog entry format.
  • All tests are passing.
  • The new code doesn't generate RuboCop offenses.
  • The PR relates to only one subject with a clear title
    and description in grammatically correct, complete sentences.

The DuplicatedGem cop checks for duplicate gem entries in Gemfiles.

Show outdated Hide outdated lib/rubocop/cop/lint/duplicated_gem.rb
private
def from_gemfile?(processed_source)
File.basename(processed_source.path) == 'Gemfile'

This comment has been minimized.

@bbatsov

bbatsov Oct 17, 2016

Collaborator

You can just use configuration to make a cop apply only to the Gemfile. Seems cleaner to me.

@bbatsov

bbatsov Oct 17, 2016

Collaborator

You can just use configuration to make a cop apply only to the Gemfile. Seems cleaner to me.

This comment has been minimized.

@maxjacobson

maxjacobson Oct 17, 2016

Contributor

Also worth noting that there are some other valid names, such as gems.rb

@maxjacobson

maxjacobson Oct 17, 2016

Contributor

Also worth noting that there are some other valid names, such as gems.rb

This comment has been minimized.

@rrosenblum

rrosenblum Oct 19, 2016

Contributor

Agreed, it would also be beneficial to able to scan things such as the Gemfile.local in the RuboCop project. Other gems follow similar structures for testing against multiple version of certain gems such as Rails. Bullet has a good example of this.

@rrosenblum

rrosenblum Oct 19, 2016

Contributor

Agreed, it would also be beneficial to able to scan things such as the Gemfile.local in the RuboCop project. Other gems follow similar structures for testing against multiple version of certain gems such as Rails. Bullet has a good example of this.

@Drenmi

I'm thinking ... if we're planning a bunch of Gemfile specific cops, should we keep them in another cop group? Maybe Bundler? @bbatsov?

Show outdated Hide outdated lib/rubocop/cop/lint/duplicated_gem.rb
def investigate(processed_source)
return unless from_gemfile?(processed_source)
gem_requirements(processed_source.ast)

This comment has been minimized.

@Drenmi

Drenmi Oct 18, 2016

Collaborator

Instantiating the hash here, and mutating it by reference from other methods makes this code very hard for me to follow.

@Drenmi

Drenmi Oct 18, 2016

Collaborator

Instantiating the hash here, and mutating it by reference from other methods makes this code very hard for me to follow.

Show outdated Hide outdated lib/rubocop/cop/lint/duplicated_gem.rb
File.basename(processed_source.path) == 'Gemfile'
end
def gem_requirements(ast)

This comment has been minimized.

@Drenmi

Drenmi Oct 18, 2016

Collaborator

Maybe this should be named #gem_declarations? When saying "requirements", it makes me think about the version constraints. 😀

@Drenmi

Drenmi Oct 18, 2016

Collaborator

Maybe this should be named #gem_declarations? When saying "requirements", it makes me think about the version constraints. 😀

This comment has been minimized.

@jmks

jmks Oct 24, 2016

Contributor

Requirements definitely seeped in from reading the bundler docs 😄

@jmks

jmks Oct 24, 2016

Contributor

Requirements definitely seeped in from reading the bundler docs 😄

Show outdated Hide outdated lib/rubocop/cop/lint/duplicated_gem.rb
ast.each_descendant.select { |e| gem_method?(e) }
end
def extract_gem_name(gem_method_node)

This comment has been minimized.

@Drenmi

Drenmi Oct 18, 2016

Collaborator

I think the extract_ prefix doesn't add anything to this method name. WDYT? 😀

@Drenmi

Drenmi Oct 18, 2016

Collaborator

I think the extract_ prefix doesn't add anything to this method name. WDYT? 😀

Show outdated Hide outdated lib/rubocop/cop/lint/duplicated_gem.rb
end
def_node_matcher :gem_method?, <<-PATTERN
[(:send, nil, :gem, ...)]

This comment has been minimized.

@Drenmi

Drenmi Oct 18, 2016

Collaborator

Accidentally got four spaces here. Also would consider renaming the matcher to #gem_declaration?. 😀

@Drenmi

Drenmi Oct 18, 2016

Collaborator

Accidentally got four spaces here. Also would consider renaming the matcher to #gem_declaration?. 😀

Show outdated Hide outdated lib/rubocop/rspec/cop_helper.rb
@@ -12,6 +12,12 @@ def inspect_source_file(cop, source)
Tempfile.open('tmp') { |f| inspect_source(cop, source, f) }
end
def inspect_gemfile(cop, source)
Tempfile.open('tmp') do |_|

This comment has been minimized.

@Drenmi

Drenmi Oct 18, 2016

Collaborator

If the argument is not needed, can drop it?

@Drenmi

Drenmi Oct 18, 2016

Collaborator

If the argument is not needed, can drop it?

This comment has been minimized.

@jmks

jmks Oct 24, 2016

Contributor

Yes. Apparently, I'm not using the Tempfile at all!

@jmks

jmks Oct 24, 2016

Contributor

Yes. Apparently, I'm not using the Tempfile at all!

Show outdated Hide outdated lib/rubocop/cop/lint/duplicated_gem.rb
end
def_node_matcher :gem_method?, <<-PATTERN
[(:send, nil, :gem, ...)]

This comment has been minimized.

@backus

backus Oct 18, 2016

Contributor

I believe you can drop the brackets here. The brackets are for specifying multiple conditions which must all be true: node pattern documentation

@backus

backus Oct 18, 2016

Contributor

I believe you can drop the brackets here. The brackets are for specifying multiple conditions which must all be true: node pattern documentation

Show outdated Hide outdated lib/rubocop/cop/lint/duplicated_gem.rb
)
end
def_node_matcher :gem_method?, <<-PATTERN

This comment has been minimized.

@backus

backus Oct 18, 2016

Contributor

You can probably just use def_node_search here and drop the ast.each_descendant stuff above

@backus

backus Oct 18, 2016

Contributor

You can probably just use def_node_search here and drop the ast.each_descendant stuff above

This comment has been minimized.

@jmks

jmks Oct 24, 2016

Contributor

Neat!
Your example pattern seems to match the tests with or without the leading $. I'm not sure what the arbitrary matching on the capture means.

@jmks

jmks Oct 24, 2016

Contributor

Neat!
Your example pattern seems to match the tests with or without the leading $. I'm not sure what the arbitrary matching on the capture means.

@backus

This comment has been minimized.

Show comment
Hide comment
@backus

backus Oct 18, 2016

Contributor

@jmks embrace the power of the NodePattern. This amount of code can get you all of the first occurrences of duplicated gems in a gemfile:

def_node_search :gems, '$(send nil :gem str ...)'

def investigate(processed_source)
  duplicated_gems =
    gems(processed_source.ast)
      .group_by { |node| node.method_args.first }
      .select   { |_, nodes| nodes.size > 1     }
      .map      { |_, nodes| nodes.first        }
end
Contributor

backus commented Oct 18, 2016

@jmks embrace the power of the NodePattern. This amount of code can get you all of the first occurrences of duplicated gems in a gemfile:

def_node_search :gems, '$(send nil :gem str ...)'

def investigate(processed_source)
  duplicated_gems =
    gems(processed_source.ast)
      .group_by { |node| node.method_args.first }
      .select   { |_, nodes| nodes.size > 1     }
      .map      { |_, nodes| nodes.first        }
end
Show outdated Hide outdated spec/rubocop/cop/lint/duplicated_gem_spec.rb
it "references gem's first occurance in message" do
inspect_gemfile(cop, source)
expect(cop.offenses.first.message).to include(2.to_s)

This comment has been minimized.

@rrosenblum

rrosenblum Oct 19, 2016

Contributor

Wouldn't it make more sense to use '2' here?

@rrosenblum

rrosenblum Oct 19, 2016

Contributor

Wouldn't it make more sense to use '2' here?

@bbatsov

This comment has been minimized.

Show comment
Hide comment
@bbatsov

bbatsov Oct 19, 2016

Collaborator

Yeah, using a separate cop group (e.g. Bundler) would be best. Those are definitely not regular style/lint cops.

Collaborator

bbatsov commented Oct 19, 2016

Yeah, using a separate cop group (e.g. Bundler) would be best. Those are definitely not regular style/lint cops.

Show outdated Hide outdated lib/rubocop/cop/lint/duplicated_gem.rb
end
end
def offense(node, gem_name, line_of_first_occurance)

This comment has been minimized.

@tdeo

tdeo Oct 20, 2016

Contributor

I think proper spelling is occurrence.

@tdeo

tdeo Oct 20, 2016

Contributor

I think proper spelling is occurrence.

@bbatsov

This comment has been minimized.

Show comment
Hide comment
@bbatsov

bbatsov Oct 22, 2016

Collaborator

@jmks ping :-)

Collaborator

bbatsov commented Oct 22, 2016

@jmks ping :-)

@jmks

This comment has been minimized.

Show comment
Hide comment
@jmks

jmks Oct 22, 2016

Contributor

Sorry, got busy this week. I'll make changes in the next couple days.

Thanks for all the great feedback!

Contributor

jmks commented Oct 22, 2016

Sorry, got busy this week. I'll make changes in the next couple days.

Thanks for all the great feedback!

@bbatsov

This comment has been minimized.

Show comment
Hide comment
@bbatsov

bbatsov Oct 24, 2016

Collaborator

@jmks OK

Collaborator

bbatsov commented Oct 24, 2016

@jmks OK

@jmks

This comment has been minimized.

Show comment
Hide comment
@jmks

jmks Oct 24, 2016

Contributor

I've updated many of the issues raised, but still looking into:

  • handling gems.rb as alternative to Gemfile
  • gem_exec of another file (e.g. Gemfile.local)
Contributor

jmks commented Oct 24, 2016

I've updated many of the issues raised, but still looking into:

  • handling gems.rb as alternative to Gemfile
  • gem_exec of another file (e.g. Gemfile.local)
@bbatsov

This comment has been minimized.

Show comment
Hide comment
@bbatsov

bbatsov Oct 30, 2016

Collaborator

handling gems.rb as alternative to Gemfile

Just add this to the Include - it accepts an array of patters.

Collaborator

bbatsov commented Oct 30, 2016

handling gems.rb as alternative to Gemfile

Just add this to the Include - it accepts an array of patters.

@bbatsov

This comment has been minimized.

Show comment
Hide comment
@bbatsov

bbatsov Oct 30, 2016

Collaborator

gem_exec of another file (e.g. Gemfile.local)

We can think about this down the road I guess.

Collaborator

bbatsov commented Oct 30, 2016

gem_exec of another file (e.g. Gemfile.local)

We can think about this down the road I guess.

@bbatsov bbatsov referenced this pull request Oct 30, 2016

Merged

[#3600] Add new `Bundler/OrderedGems` cop #3657

11 of 11 tasks complete
[#3600] Add new `Bundler/DuplicatedGem` cop
The DuplicatedGem cop checks for duplicate gem entries in Gemfiles.
@jmks

This comment has been minimized.

Show comment
Hide comment
@jmks

jmks Nov 1, 2016

Contributor

Updated!

gem_exec of another file (e.g. Gemfile.local)

A couple simple cases of eval_gemfile could be done statically where the parsed source is pretty simple:

eval_gemfile 'Gemfile.local'

# and

local_gemfile = 'Gemfile.local'
eval_gemfile local_gemfile if File.exist?(local_gemfile)

But they could be added later like you suggest.

Contributor

jmks commented Nov 1, 2016

Updated!

gem_exec of another file (e.g. Gemfile.local)

A couple simple cases of eval_gemfile could be done statically where the parsed source is pretty simple:

eval_gemfile 'Gemfile.local'

# and

local_gemfile = 'Gemfile.local'
eval_gemfile local_gemfile if File.exist?(local_gemfile)

But they could be added later like you suggest.

@jmks jmks changed the title from [#3600] Add new `Lint/DuplicatedGem` cop to [#3600] Add new `Bundler/DuplicatedGem` cop Nov 1, 2016

@bbatsov bbatsov merged commit 58a7098 into rubocop-hq:master Nov 1, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment