Bump rubocop to use `v0.50.x` #6368

ashmaroli · 2017-09-15T07:44:15Z


        bump and configure to use Rubocop 0.50.x


        apply corrections as suggested by Rubocop


        partially revert to previous iteration

pathawks

Thanks for taking the time to do this 🍻👍

pathawks · 2017-09-15T14:15:45Z

lib/jekyll/commands/doctor.rb

@@ -86,7 +86,7 @@ def fsnotify_buggy?(_site)
        def urls_only_differ_by_case(site)
          urls_only_differ_by_case = false
          urls = case_insensitive_urls(site.pages + site.docs_to_write, site.dest)
-          urls.each do |_case_insensitive_url, real_urls|
+          urls.each_value do |real_urls|


parkr

Nice! One code update suggestion and some thoughts on the configuration changes. Instead of turning them off, it might be better to exclude certain files if we decide that the rule is helpful (most are).

parkr · 2017-09-15T14:57:04Z

lib/jekyll/converters/markdown/kramdown_parser.rb

@@ -42,12 +42,14 @@ def convert(content)
        end

        private
+        # rubocop:disable Performance/HashEachMethods
        def make_accessible(hash = @config)
          hash.keys.each do |key|


Can we use each_key here?

each_key broke our test-suite..

parkr · 2017-09-15T14:57:04Z

.rubocop.yml

@@ -82,6 +86,12 @@ Metrics/ParameterLists:
  Max: 4
 Metrics/PerceivedComplexity:
  Max: 8
+Naming/FileName:
+  Enabled: false


What fails for this one?

From their docs,

This cop makes sure that Ruby source files have snake_case names. Ruby scripts (i.e. source files with a shebang in the first line) are ignored.

and flags us for the following:
- 'Gemfile'
- 'Rakefile'
- 'lib/theme_template/Gemfile'
- 'test/fixtures/test-dependency-theme/test-dependency-theme.gemspec'
- 'test/fixtures/test-theme/test-theme.gemspec'

which IMO is a bug because this cop existed as an enabled-by-default-cop earlier known as Style/FileName

Yeah... that looks like a bug. Weird.

parkr · 2017-09-15T14:57:04Z

.rubocop.yml

+Naming/FileName:
+  Enabled: false
+Naming/HeredocDelimiterCase:
+  Enabled: false


We should enable this!

okay.. I'll go with enforcing uppercase (default)

parkr · 2017-09-15T14:57:04Z

.rubocop.yml

+Naming/HeredocDelimiterCase:
+  Enabled: false
+Naming/HeredocDelimiterNaming:
+  Enabled: false


What fails for this?

'features/support/helpers.rb'

'test/test_redcarpet.rb'

'test/test_tags.rb'

are flagged for presence of EOS as the delimiter

parkr · 2017-09-15T14:57:04Z

.rubocop.yml

@@ -109,7 +119,7 @@ Style/Documentation:
    - !ruby/regexp /features\/.*.rb$/
 Style/DoubleNegation:
  Enabled: false
-Style/FileName:
+Style/Encoding:


Can we just exclude problematic files?

This cop flags files that contain the following magic comment because from Ruby 2.0+, UTF-8 is the default source file encoding:

# encoding: UTF-8

So, IMO, we switch to when_needed if we're to enable the cop -- only enforce an encoding comment if there are non-ASCII chars

Let's do when_needed, then!

parkr · 2017-09-15T14:57:04Z

.rubocop.yml

 Lint/UnreachableCode:
  Severity: error
 Lint/UselessAccessModifier:
  Enabled: false
+Lint/Void:
+  Enabled: false


What even is this? 😂

This cop checks for the use of a return with a value in a context where it will be ignored.
In our case specifically, this cop (incorrectly ?) flags Site#config= similar to the documented example

Oh we must return some value and it doesn't like that! We can enable it in a subsequent pass.

parkr · 2017-09-15T14:57:04Z

.rubocop.yml

@@ -44,10 +44,14 @@ Layout/SpaceInsideBrackets:
  Enabled: false
 Lint/EndAlignment:
  Severity: error
+Lint/RescueWithoutErrorClass:
+  Enabled: false


Oooo we should enable this.

I wasn't sure about it though, since unnamed rescue means rescuing StandardError by default.

Yeah, but it's always better to rescue explicitly what you want to rescue instead of everything. We can do this in a subsequent pass.

parkr · 2017-09-15T14:57:04Z

.rubocop.yml

@@ -21,7 +21,7 @@ Layout/EmptyLinesAroundAccessModifier:
 Layout/EmptyLinesAroundModuleBody:
  Enabled: false
 Layout/EndOfLine:
-  EnforcedStyle: lf
+  EnforcedStyle: native


Done we want lf here?

Technically, we do.. but if a developer on Windows were to run script/fmt they'd unnecessarily get taxed by Rubocop. In my own experience, it has been irritating..
A properly configured Git on Windows automatically indexes files compatible for cross-platform use.


        enforce Heredocs with uppercase delimiters


        Merge branch 'master' of https://github.com/jekyll/jekyll into bump-r…

…ubocop


        regulate magic-comment for UTF-8 encoding

pathawks · 2017-09-22T13:06:31Z

@jekyllbot: merge +dev


        Update history to reflect merge of #6368 [ci skip]

ashmaroli added 3 commits Sep 15, 2017

bump and configure to use Rubocop 0.50.x

b16ec49

apply corrections as suggested by Rubocop

Loading status checks…

deae2c7

partially revert to previous iteration

Loading status checks…

076c9b8

pathawks approved these changes Sep 15, 2017

View changes

parkr reviewed Sep 15, 2017

View changes

ashmaroli added 3 commits Sep 15, 2017

enforce Heredocs with uppercase delimiters

Loading status checks…

6e77120

Merge branch 'master' of https://github.com/jekyll/jekyll into bump-r…

e57c09e

…ubocop

regulate magic-comment for UTF-8 encoding

Loading status checks…

36178e9

DirtyF approved these changes Sep 22, 2017

View changes

jekyllbot merged commit 00bad8b into jekyll:master Sep 22, 2017
2 checks passed

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

continuous-integration/travis-ci/pr The Travis CI build passed
Details

jekyllbot added fix internal labels Sep 22, 2017

jekyllbot added a commit that referenced this pull request Sep 22, 2017

Update history to reflect merge of #6368 [ci skip]

Loading status checks…

c84aef0

ashmaroli deleted the ashmaroli:bump-rubocop branch Sep 22, 2017

jekyll locked and limited conversation to collaborators Jul 12, 2019

jekyllbot added the frozen-due-to-age label Jul 12, 2019

jekyll/jekyll

Bump rubocop to use `v0.50.x` #6368

Bump rubocop to use `v0.50.x` #6368

ashmaroli commented Sep 15, 2017

pathawks left a comment

This comment has been minimized.

parkr left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

pathawks commented Sep 22, 2017

Sponsor jekyll/jekyll

jekyll/jekyll

Join GitHub today

Bump rubocop to use `v0.50.x` #6368

Conversation

ashmaroli commented Sep 15, 2017

pathawks left a comment

This comment has been minimized.

parkr left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

pathawks commented Sep 22, 2017