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

Bump rubocop to use v0.50.x #6368

Merged
merged 6 commits into from Sep 22, 2017
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
14 changes: 12 additions & 2 deletions .rubocop.yml
Expand Up @@ -21,7 +21,7 @@ Layout/EmptyLinesAroundAccessModifier:
Layout/EmptyLinesAroundModuleBody:
Enabled: false
Layout/EndOfLine:
EnforcedStyle: lf
EnforcedStyle: native
Copy link
Member

Choose a reason for hiding this comment

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

Done we want lf here?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

👍

Layout/ExtraSpacing:
AllowForAlignment: true
Layout/FirstParameterIndentation:
Expand All @@ -44,10 +44,14 @@ Layout/SpaceInsideBrackets:
Enabled: false
Lint/EndAlignment:
Severity: error
Lint/RescueWithoutErrorClass:
Enabled: false
Copy link
Member

Choose a reason for hiding this comment

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

Oooo we should enable this.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Lint/UnreachableCode:
Severity: error
Lint/UselessAccessModifier:
Enabled: false
Lint/Void:
Enabled: false
Copy link
Member

Choose a reason for hiding this comment

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

What even is this? 😂

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Metrics/AbcSize:
Max: 21
Metrics/BlockLength:
Expand Down Expand Up @@ -82,6 +86,12 @@ Metrics/ParameterLists:
Max: 4
Metrics/PerceivedComplexity:
Max: 8
Naming/FileName:
Enabled: false
Copy link
Member

Choose a reason for hiding this comment

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

What fails for this one?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Naming/HeredocDelimiterCase:
Enabled: false
Copy link
Member

Choose a reason for hiding this comment

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

We should enable this!

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Naming/HeredocDelimiterNaming:
Enabled: false
Copy link
Member

Choose a reason for hiding this comment

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

What fails for this?

Copy link
Member Author

Choose a reason for hiding this comment

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

  • 'features/support/helpers.rb'
  • 'test/test_redcarpet.rb'
  • 'test/test_tags.rb'

are flagged for presence of EOS as the delimiter

Security/MarshalLoad:
Exclude:
- !ruby/regexp /test\/.*.rb$/
Expand Down Expand Up @@ -109,7 +119,7 @@ Style/Documentation:
- !ruby/regexp /features\/.*.rb$/
Style/DoubleNegation:
Enabled: false
Style/FileName:
Style/Encoding:
Copy link
Member

Choose a reason for hiding this comment

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

Can we just exclude problematic files?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Let's do when_needed, then!

Enabled: false
Style/GuardClause:
Enabled: false
Expand Down
2 changes: 1 addition & 1 deletion Gemfile
Expand Up @@ -30,7 +30,7 @@ group :test do
gem "nokogiri", RUBY_VERSION >= "2.2" ? "~> 1.7" : "~> 1.7.0"
gem "rspec"
gem "rspec-mocks"
gem "rubocop", "~> 0.49.1"
gem "rubocop", "~> 0.50.0"
gem "test-dependency-theme", :path => File.expand_path("test/fixtures/test-dependency-theme", __dir__)
gem "test-theme", :path => File.expand_path("test/fixtures/test-theme", __dir__)

Expand Down
4 changes: 2 additions & 2 deletions lib/jekyll.rb
Expand Up @@ -119,15 +119,15 @@ def configuration(override = {})
# timezone - the IANA Time Zone
#
# Returns nothing
# rubocop:disable Style/AccessorMethodName
# rubocop:disable Naming/AccessorMethodName
def set_timezone(timezone)
ENV["TZ"] = if Utils::Platforms.really_windows?
Utils::WinTZ.calculate(timezone)
else
timezone
end
end
# rubocop:enable Style/AccessorMethodName
# rubocop:enable Naming/AccessorMethodName

# Public: Fetch the logger instance for this Jekyll process.
#
Expand Down
2 changes: 1 addition & 1 deletion lib/jekyll/commands/doctor.rb
Expand Up @@ -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|
Copy link
Member

Choose a reason for hiding this comment

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

Cool!

next unless real_urls.uniq.size > 1
urls_only_differ_by_case = true
Jekyll.logger.warn "Warning:", "The following URLs only differ" \
Expand Down
2 changes: 1 addition & 1 deletion lib/jekyll/commands/serve/servlet.rb
Expand Up @@ -27,7 +27,7 @@ def search_file(req, res, basename)
super || super(req, res, ".html") || super(req, res, "#{basename}.html")
end

# rubocop:disable Style/MethodName
# rubocop:disable Naming/MethodName
def do_GET(req, res)
rtn = super
validate_and_ensure_charset(req, res)
Expand Down
4 changes: 2 additions & 2 deletions lib/jekyll/converters/markdown.rb
Expand Up @@ -27,7 +27,7 @@ def setup
# Rubocop does not allow reader methods to have names starting with `get_`
# To ensure compatibility, this check has been disabled on this method
#
# rubocop:disable Style/AccessorMethodName
# rubocop:disable Naming/AccessorMethodName
def get_processor
case @config["markdown"].downcase
when "redcarpet" then return RedcarpetParser.new(@config)
Expand All @@ -37,7 +37,7 @@ def get_processor
custom_processor
end
end
# rubocop:enable Style/AccessorMethodName
# rubocop:enable Naming/AccessorMethodName

# Public: Provides you with a list of processors, the ones we
# support internally and the ones that you have provided to us (if you
Expand Down
2 changes: 2 additions & 0 deletions lib/jekyll/converters/markdown/kramdown_parser.rb
Expand Up @@ -42,12 +42,14 @@ def convert(content)
end

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

Choose a reason for hiding this comment

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

Can we use each_key here?

Copy link
Member Author

Choose a reason for hiding this comment

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

each_key broke our test-suite..

hash[key.to_sym] = hash[key]
make_accessible(hash[key]) if hash[key].is_a?(Hash)
end
end
# rubocop:enable Performance/HashEachMethods

# config[kramdown][syntax_higlighter] >
# config[kramdown][enable_coderay] >
Expand Down
2 changes: 1 addition & 1 deletion lib/jekyll/reader.rb
Expand Up @@ -25,7 +25,7 @@ def read

# Sorts posts, pages, and static files.
def sort_files!
site.collections.values.each { |c| c.docs.sort! }
site.collections.each_value { |c| c.docs.sort! }
site.pages.sort_by!(&:name)
site.static_files.sort_by!(&:relative_path)
end
Expand Down
2 changes: 1 addition & 1 deletion lib/jekyll/readers/collection_reader.rb
Expand Up @@ -14,7 +14,7 @@ def initialize(site)
#
# Returns nothing.
def read
site.collections.each do |_, collection|
site.collections.each_value do |collection|
collection.read unless SPECIAL_COLLECTIONS.include?(collection.label)
end
end
Expand Down
14 changes: 7 additions & 7 deletions lib/jekyll/site.rb
Expand Up @@ -85,11 +85,11 @@ def print_stats
#
# Returns nothing
def reset
if config["time"]
self.time = Utils.parse_date(config["time"].to_s, "Invalid time in _config.yml.")
else
self.time = Time.now
end
self.time = if config["time"]
Utils.parse_date(config["time"].to_s, "Invalid time in _config.yml.")
else
Time.now
end
self.layouts = {}
self.pages = []
self.static_files = []
Expand Down Expand Up @@ -238,7 +238,7 @@ def post_attr_hash(post_attr)
posts.docs.each do |p|
p.data[post_attr].each { |t| hash[t] << p } if p.data[post_attr]
end
hash.values.each { |posts| posts.sort!.reverse! }
hash.each_value { |posts| posts.sort!.reverse! }
hash
end

Expand Down Expand Up @@ -449,7 +449,7 @@ def configure_file_read_opts

private
def render_docs(payload)
collections.each do |_, collection|
collections.each_value do |collection|
collection.docs.each do |document|
if regenerator.regenerate?(document)
document.output = Jekyll::Renderer.new(self, document, payload).run
Expand Down
2 changes: 1 addition & 1 deletion test/test_ansi.rb
Expand Up @@ -8,7 +8,7 @@ class TestAnsi < JekyllUnitTest
@subject = Jekyll::Utils::Ansi
end

Jekyll::Utils::Ansi::COLORS.each do |color, _val|
Jekyll::Utils::Ansi::COLORS.each_key do |color|
should "respond_to? #{color}" do
assert @subject.respond_to?(color)
end
Expand Down