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

Fix remaining RuboCop issues #3765

Merged

Conversation

utkarsh2102
Copy link
Contributor

@utkarsh2102 utkarsh2102 commented Jun 30, 2020

With #3731 and #3740 merged, this covers up the
remaining part of the issues.
This was discovered when one tries to create a gem
with a different framework.
Could be reproduced with:
bundle gem foo --ext --test=minitest --rubocop

Signed-off-by: Utkarsh Gupta <utkarsh@debian.org>


Tasks:

  • Describe the problem / feature
  • Write tests
  • Write code to solve the problem
  • Get code review from coworkers / friends

I will abide by the code of conduct.

With rubygems#3731 and rubygems#3740 merged, this covers up the
remaining part of the issues.
This was discovered when one tries to create a gem
with a different framework.
Could be reproduced with:
`bundle gem foo --ext --test=test-unit`

Signed-off-by: Utkarsh Gupta <utkarsh@debian.org>
@utkarsh2102 utkarsh2102 marked this pull request as draft June 30, 2020 07:53
In case of multiple Rake tasks, the default tasks would
look something like this:
`task default: [:spec, :rubocop]`

Instead, they should use %i and look something like this:
`task default: %i[spec rubocop]`

Signed-off-by: Utkarsh Gupta <utkarsh@debian.org>
If the blank lines aren't used, then rubocop tries to
sort them in alphabetical order within their section.
Thus, adding lines so rubocop considers them as
different sections and doesn't try to sort them.

Signed-off-by: Utkarsh Gupta <utkarsh@debian.org>
@utkarsh2102 utkarsh2102 marked this pull request as ready for review June 30, 2020 08:31
Copy link
Member

@deivid-rodriguez deivid-rodriguez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test failures seem legit. To make sure the gems are in different groups, but inside the conditionals. Otherwise they add up regardless of options and we'll end up with multiple blank lines when, for example, config[:rubocop] is true and either config[:test] or config[:ext] is false.

So do somethin like the following instead:

diff --git a/bundler/lib/bundler/templates/newgem/Gemfile.tt b/bundler/lib/bundler/templates/newgem/Gemfile.tt
index 82504f92d4..1d55fd7a90 100644
--- a/bundler/lib/bundler/templates/newgem/Gemfile.tt
+++ b/bundler/lib/bundler/templates/newgem/Gemfile.tt
@@ -7,13 +7,14 @@ gemspec
 
 gem "rake", "~> 13.0"
 <%- if config[:ext] -%>
+
 gem "rake-compiler"
 <%- end -%>
-
 <%- if config[:test] -%>
+
 gem "<%= config[:test] %>", "~> <%= config[:test_framework_version] %>"
 <%- end -%>
-
 <%- if config[:rubocop] -%>
+
 gem "rubocop", "~> 0.80"
 <%- end -%>

The Gemfile wasn't properly put in the last commit.
As a result, Layout/EmptyLines inspected an offense
in the Gemfile.

This also fixes the spec w.r.t change in the task
default.

Signed-off-by: Utkarsh Gupta <utkarsh@debian.org>
Copy link
Member

@deivid-rodriguez deivid-rodriguez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ideally we would extract the spec you added in the previous PR to a shared example, and reuse it for the different flag combinations. This is already done for several things in bundle gem specs. Would you like to give it a try?

@utkarsh2102
Copy link
Contributor Author

I ideally we would extract the spec you added in the previous PR to a shared example, and reuse it for the different flag combinations. This is already done for several things in bundle gem specs. Would you like to give it a try?

Hi @deivid-rodriguez,
I already have and I think I have messed up something else in turn :P
(that's why I didn't push 😅)

Since this PR was made because we missed checking
RuboCop offenses with different flags, therefore
adding tests so that all flag combinations are
tested.

Signed-off-by: Utkarsh Gupta <utkarsh@debian.org>
The newly added specs needs to be tagged as
:readline, otherwise they fail on Windows with
the backtrace: `ZeroDivisionError: divided by 0`.

Such issues are already being skipped on Windows.

Signed-off-by: Utkarsh Gupta <utkarsh@debian.org>
Copy link
Member

@deivid-rodriguez deivid-rodriguez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thanks!

@deivid-rodriguez deivid-rodriguez merged commit e123287 into rubygems:master Jun 30, 2020
@utkarsh2102 utkarsh2102 deleted the fix-remaining-rubocop-issues branch July 2, 2020 00:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants