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

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

Merged
merged 1 commit into from Nov 1, 2016

Conversation

@jmks
Copy link
Contributor

@jmks 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.

private

def from_gemfile?(processed_source)
File.basename(processed_source.path) == 'Gemfile'
Copy link
Collaborator

@bbatsov bbatsov Oct 17, 2016

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

Copy link
Contributor

@maxjacobson maxjacobson Oct 17, 2016

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

Copy link
Contributor

@rrosenblum rrosenblum Oct 19, 2016

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.

Copy link
Collaborator

@Drenmi Drenmi left a comment

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

def investigate(processed_source)
return unless from_gemfile?(processed_source)

gem_requirements(processed_source.ast)
Copy link
Collaborator

@Drenmi Drenmi Oct 18, 2016

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

File.basename(processed_source.path) == 'Gemfile'
end

def gem_requirements(ast)
Copy link
Collaborator

@Drenmi Drenmi Oct 18, 2016

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

Copy link
Contributor Author

@jmks jmks Oct 24, 2016

Requirements definitely seeped in from reading the bundler docs 😄

ast.each_descendant.select { |e| gem_method?(e) }
end

def extract_gem_name(gem_method_node)
Copy link
Collaborator

@Drenmi Drenmi Oct 18, 2016

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

end

def_node_matcher :gem_method?, <<-PATTERN
[(:send, nil, :gem, ...)]
Copy link
Collaborator

@Drenmi Drenmi Oct 18, 2016

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

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

@Drenmi Drenmi Oct 18, 2016

If the argument is not needed, can drop it?

Copy link
Contributor Author

@jmks jmks Oct 24, 2016

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

end

def_node_matcher :gem_method?, <<-PATTERN
[(:send, nil, :gem, ...)]
Copy link
Contributor

@backus backus Oct 18, 2016

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

)
end

def_node_matcher :gem_method?, <<-PATTERN
Copy link
Contributor

@backus backus Oct 18, 2016

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

Copy link
Contributor Author

@jmks jmks Oct 24, 2016

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

@backus 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


it "references gem's first occurance in message" do
inspect_gemfile(cop, source)
expect(cop.offenses.first.message).to include(2.to_s)
Copy link
Contributor

@rrosenblum rrosenblum Oct 19, 2016

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

@bbatsov
Copy link
Collaborator

@bbatsov 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.

end
end

def offense(node, gem_name, line_of_first_occurance)
Copy link
Contributor

@tdeo tdeo Oct 20, 2016

I think proper spelling is occurrence.

@bbatsov
Copy link
Collaborator

@bbatsov bbatsov commented Oct 22, 2016

@jmks ping :-)

@jmks
Copy link
Contributor Author

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

@bbatsov bbatsov commented Oct 24, 2016

@jmks OK

@jmks
Copy link
Contributor Author

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

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

@bbatsov bbatsov commented Oct 30, 2016

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

We can think about this down the road I guess.

The DuplicatedGem cop checks for duplicate gem entries in Gemfiles.
@jmks jmks force-pushed the gemfile_gem_requirement_only_once_cop branch from 6b20abc to 3ec5906 Nov 1, 2016
@jmks
Copy link
Contributor Author

@jmks 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 [#3600] Add new Lint/DuplicatedGem cop [#3600] Add new Bundler/DuplicatedGem cop Nov 1, 2016
@bbatsov bbatsov merged commit 58a7098 into rubocop:master Nov 1, 2016
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants