Conversation
pathawks
left a comment
There was a problem hiding this comment.
Thanks for taking the time to do this 🍻👍
| 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
left a comment
There was a problem hiding this comment.
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).
| private | ||
| # rubocop:disable Performance/HashEachMethods | ||
| def make_accessible(hash = @config) | ||
| hash.keys.each do |key| |
There was a problem hiding this comment.
each_key broke our test-suite..
| Metrics/PerceivedComplexity: | ||
| Max: 8 | ||
| Naming/FileName: | ||
| Enabled: false |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Yeah... that looks like a bug. Weird.
| Naming/FileName: | ||
| Enabled: false | ||
| Naming/HeredocDelimiterCase: | ||
| Enabled: false |
There was a problem hiding this comment.
okay.. I'll go with enforcing uppercase (default)
| Naming/HeredocDelimiterCase: | ||
| Enabled: false | ||
| Naming/HeredocDelimiterNaming: | ||
| Enabled: false |
There was a problem hiding this comment.
- 'features/support/helpers.rb'
- 'test/test_redcarpet.rb'
- 'test/test_tags.rb'
are flagged for presence of EOS as the delimiter
| Style/DoubleNegation: | ||
| Enabled: false | ||
| Style/FileName: | ||
| Style/Encoding: |
There was a problem hiding this comment.
Can we just exclude problematic files?
There was a problem hiding this comment.
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-8So, IMO, we switch to when_needed if we're to enable the cop -- only enforce an encoding comment if there are non-ASCII chars
| Lint/UselessAccessModifier: | ||
| Enabled: false | ||
| Lint/Void: | ||
| Enabled: false |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Oh we must return some value and it doesn't like that! We can enable it in a subsequent pass.
| Lint/EndAlignment: | ||
| Severity: error | ||
| Lint/RescueWithoutErrorClass: | ||
| Enabled: false |
There was a problem hiding this comment.
I wasn't sure about it though, since unnamed rescue means rescuing StandardError by default.
There was a problem hiding this comment.
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.
| Enabled: false | ||
| Layout/EndOfLine: | ||
| EnforcedStyle: lf | ||
| EnforcedStyle: native |
There was a problem hiding this comment.
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.
|
@jekyllbot: merge +dev |
Fixes #6367