Skip to content

Fix remaining RuboCop issues#3765

Merged
deivid-rodriguez merged 6 commits intoruby:masterfrom
utkarsh2102:fix-remaining-rubocop-issues
Jun 30, 2020
Merged

Fix remaining RuboCop issues#3765
deivid-rodriguez merged 6 commits intoruby:masterfrom
utkarsh2102:fix-remaining-rubocop-issues

Conversation

@utkarsh2102
Copy link
Copy Markdown
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 ruby#3731 and ruby#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
Copy Markdown
Contributor

@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
Copy Markdown
Contributor

@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
Copy Markdown
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>
@utkarsh2102 utkarsh2102 force-pushed the fix-remaining-rubocop-issues branch from b42562c to d08250e Compare June 30, 2020 17:06
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
Copy Markdown
Contributor

@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 ruby: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.

3 participants