Trying to appease Rubocop #4301

Merged
merged 37 commits into from Jan 4, 2016

Conversation

Projects
None yet
4 participants
@pathawks
Member

pathawks commented Dec 30, 2015

Am going through this list of style issues and tackling the low hanging fruit. If this is productive, I may continue.

This is mostly for my own learning.

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Dec 30, 2015

Contributor

Do you plan on doing more @pathawks because as far as I'm concerned as it stands right now this is ready to be merged and perfect!

Contributor

envygeeks commented Dec 30, 2015

Do you plan on doing more @pathawks because as far as I'm concerned as it stands right now this is ready to be merged and perfect!

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Dec 30, 2015

Contributor

Well... asked too late since as I was asking you did more lol.

Contributor

envygeeks commented Dec 30, 2015

Well... asked too late since as I was asking you did more lol.

@pathawks

This comment has been minimized.

Show comment
Hide comment
@pathawks

pathawks Dec 30, 2015

Member

Well... asked too late since as I was asking you did more lol.

lol
Yes, I plan to work on this a bit more over the next couple of days. There are some failing tests that I need to investigate :rage4:

Member

pathawks commented Dec 30, 2015

Well... asked too late since as I was asking you did more lol.

lol
Yes, I plan to work on this a bit more over the next couple of days. There are some failing tests that I need to investigate :rage4:

@pathawks

This comment has been minimized.

Show comment
Hide comment
@pathawks

pathawks Dec 30, 2015

Member

Failing tests seem unrelated. Drops have no method key?, which caused a wide variety of problems... :goberserk:

Member

pathawks commented Dec 30, 2015

Failing tests seem unrelated. Drops have no method key?, which caused a wide variety of problems... :goberserk:

@pathawks

This comment has been minimized.

Show comment
Hide comment
@pathawks

pathawks Dec 30, 2015

Member

What is the best way to do a big PR like this? Should I just keep committing and then squash+merge when I finish / get bored?
Or you could merge this batch and I’ll just open another PR for the next batch?

Member

pathawks commented Dec 30, 2015

What is the best way to do a big PR like this? Should I just keep committing and then squash+merge when I finish / get bored?
Or you could merge this batch and I’ll just open another PR for the next batch?

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Dec 30, 2015

Contributor

Squash likes but keep others as their own commits so there is a clear history. So for example you have two commits that have "key?" changes, those two should be squashed but everything else that isn't the same of another commit is easier to verify over multiple commits in this case.

Contributor

envygeeks commented Dec 30, 2015

Squash likes but keep others as their own commits so there is a clear history. So for example you have two commits that have "key?" changes, those two should be squashed but everything else that isn't the same of another commit is easier to verify over multiple commits in this case.

@pathawks

View changes

lib/jekyll/frontmatter_defaults.rb
@@ -130,7 +130,7 @@ def valid?(set)
# new_scope - the new scope hash
#
# Returns true if the new scope has precedence over the older
- def has_precedence?(old_scope, new_scope)
+ def precedence?(old_scope, new_scope)

This comment has been minimized.

@pathawks

pathawks Dec 31, 2015

Member

precedence? is private, so renaming is not a breaking change.

@pathawks

pathawks Dec 31, 2015

Member

precedence? is private, so renaming is not a breaking change.

This comment has been minimized.

@envygeeks

envygeeks Dec 31, 2015

Contributor

I'm gonna caution on the side of it not being entirely clear to the user, things like Rubocop tell you to only use private once but that's an ignorant approach considering I didn't even know it was private until I scrolled up quite a ways. They also tell you to indent on that private, which just muddies the code view.

I'm not against the change though, just saying it might not be clear to a user.

@envygeeks

envygeeks Dec 31, 2015

Contributor

I'm gonna caution on the side of it not being entirely clear to the user, things like Rubocop tell you to only use private once but that's an ignorant approach considering I didn't even know it was private until I scrolled up quite a ways. They also tell you to indent on that private, which just muddies the code view.

I'm not against the change though, just saying it might not be clear to a user.

This comment has been minimized.

@pathawks

pathawks Dec 31, 2015

Member

Do you have a suggestion for making it more clear?

@pathawks

pathawks Dec 31, 2015

Member

Do you have a suggestion for making it more clear?

This comment has been minimized.

@envygeeks

envygeeks Dec 31, 2015

Contributor

For all my source, I disable Rubocops "worthless private" warning and add private above every private def no matter how many there are, so it's clear that method is private, the same way other languages enforce that kind of explicitness.

private
def method1
  #
end

private
def method2
  #
end
@envygeeks

envygeeks Dec 31, 2015

Contributor

For all my source, I disable Rubocops "worthless private" warning and add private above every private def no matter how many there are, so it's clear that method is private, the same way other languages enforce that kind of explicitness.

private
def method1
  #
end

private
def method2
  #
end

This comment has been minimized.

@envygeeks

envygeeks Dec 31, 2015

Contributor

Actually, I think I already disabled that in Jekyll too.

@envygeeks

envygeeks Dec 31, 2015

Contributor

Actually, I think I already disabled that in Jekyll too.

This comment has been minimized.

@pathawks

pathawks Dec 31, 2015

Member

Ok, I see that this convention is already implemented in most other parts of Jekyll.

@pathawks

pathawks Dec 31, 2015

Member

Ok, I see that this convention is already implemented in most other parts of Jekyll.

@pathawks

This comment has been minimized.

Show comment
Hide comment
@pathawks

pathawks Jan 1, 2016

Member

Before:

119 files inspected, 1853 offenses detected

After:

119 files inspected, 1066 offenses detected
Member

pathawks commented Jan 1, 2016

Before:

119 files inspected, 1853 offenses detected

After:

119 files inspected, 1066 offenses detected
@pathawks

This comment has been minimized.

Show comment
Hide comment
@pathawks

pathawks Jan 3, 2016

Member

master after b7d5a26:

69 files inspected, 606 offenses detected

I will weed some of these commits out when I get the chance.

Member

pathawks commented Jan 3, 2016

master after b7d5a26:

69 files inspected, 606 offenses detected

I will weed some of these commits out when I get the chance.

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Jan 3, 2016

Contributor

It's gone down slightly more after further refinement, now it sits at 533 offenses as it stands.

Contributor

envygeeks commented Jan 3, 2016

It's gone down slightly more after further refinement, now it sits at 533 offenses as it stands.

pathawks added some commits Jan 3, 2016

Rubocop: Style/EmptyLiteral
 - Use array literal [] instead of Array.new
 - Use hash literal {} instead of Hash.new
Rubocop: Style/Semicolon
 - Do not use semicolons to terminate expressions
Rubocop: Style/LineEndConcatenation
 - Use \ instead of + or << to concatenate those strings
Rubocop: Style/SpecialGlobalVars
 - Prefer $LOAD_PATH over $:
Rubocop: Style/AndOr
 - Use && instead of and
 - Use || instead of or
Rubocop: Style/PercentLiteralDelimiters
 - %w-literals should be delimited by ( and )
Rubocop: Style/WordArray
 - Use %w or %W for array of words
Rubocop: Style/DeprecatedHashMethods
 - Hash#has_key? is deprecated in favor of Hash#key?
Add method `key?` to Drop
Rubocop: Performance/StringReplacement
 - Use delete! instead of gsub!
 - Use tr instead of gsub
Rubocop: Style/HashSyntax
 - Use hash rockets syntax
Rubocop: Style/EmptyLinesAroundClassBody
 - Extra empty line detected at class body end
Rubocop: Style/SpaceAfterComma
 - Space missing after comma
Rubocop: Style/SpaceBeforeBlockBraces
Rubocop: Style/SpaceInsideBlockBraces
Rubocop: Style/NegatedIf
 - Favor unless over if for negative conditions
Rubocop: Style/SymbolProc
 - Pass &:to_sym as an argument to map instead of a block
 - Pass &:capitalize as an argument to select instead of a block
 - Pass &:to_s as an argument to map instead of a block
@pathawks

This comment has been minimized.

Show comment
Hide comment
@pathawks

pathawks Jan 4, 2016

Member

Down to 321, and I'm not sure why the test failed (only on 2.2)

Failure:
TestKramdown#test_: kramdown should render fenced code blocks with syntax highlighting.  [/home/travis/build/jekyll/jekyll/test/test_kramdown.rb:59]
Minitest::Assertion: Failed refutation, no message given
553 tests, 926 assertions, 1 failures, 0 errors, 0 skips
Member

pathawks commented Jan 4, 2016

Down to 321, and I'm not sure why the test failed (only on 2.2)

Failure:
TestKramdown#test_: kramdown should render fenced code blocks with syntax highlighting.  [/home/travis/build/jekyll/jekyll/test/test_kramdown.rb:59]
Minitest::Assertion: Failed refutation, no message given
553 tests, 926 assertions, 1 failures, 0 errors, 0 skips
@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Jan 4, 2016

Contributor

It randomly fails on 2.2 because of bugs in 2.2 that were addressed in the latest 2.2 but Travis+RVM is absolutely totally broken so 2.2 points to the first release of 2.2. To fix it we have to point it to the full version of 2.2.* which is annoying to maintain.

Contributor

envygeeks commented Jan 4, 2016

It randomly fails on 2.2 because of bugs in 2.2 that were addressed in the latest 2.2 but Travis+RVM is absolutely totally broken so 2.2 points to the first release of 2.2. To fix it we have to point it to the full version of 2.2.* which is annoying to maintain.

pathawks added some commits Jan 4, 2016

Rubocop: Style/Proc
 - Use proc instead of Proc.new
...and use lambda instead of proc
Rubocop: Lint/UnusedBlockArgument
 - Unused block argument
@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Jan 4, 2016

Member

Keep everything as individual commits, I think. I have encountered too many general "fix the style" commits for my liking.

Member

parkr commented Jan 4, 2016

Keep everything as individual commits, I think. I have encountered too many general "fix the style" commits for my liking.

pathawks added some commits Jan 4, 2016

Rubocop: Lint/StringConversionInInterpolation
 - Redundant use of Object#to_s in interpolation
Rubocop: Style/Next
 - Use next to skip iteration
Rubocop: Style/CaseIndentation
 - Indent when as deep as case
Rubocop: Style/ElseAlignment
 - Align else with if
Rubocop: Lint/EndAlignment
 - Align end with if
Rubocop: Style/TrivialAccessors
 - Use `attr_writer` to define trivial writer methods

pathawks added some commits Jan 4, 2016

Rubocop: Style/NestedTernaryOperator
 - Ternary operators must not be nested. Prefer if/else constructs instead.
Rubocop: Do not use unless with else
 - Rewrite these with the positive case first
Rubocop: Style/PerlBackrefs
 - Avoid the use of Perl-style backrefs
Rubocop: Style/BlockDelimiters
 - Avoid using {...} for multi-line blocks
Rubocop: Style/SpaceInsideRangeLiteral
 - Space inside range literal
Rubocop: Style/ParenthesesAroundCondition
 - Don't use parentheses around the condition of an if
Rubocop: Style/TrailingWhitespace
 - Trailing whitespace detected
Rubocop: Style/EmptyLines
 - Extra blank line detected
Rubocop: Style/EmptyLinesAroundBlockBody
 - Extra empty line detected at block body beginning
@@ -67,13 +65,11 @@ def build(site, options)
# options - A Hash of options passed to the command
#
# Returns nothing.
- def watch(site, options)
+ def watch(_site, options)

This comment has been minimized.

@parkr

parkr Jan 4, 2016

Member

you can make this just _, I think?

@parkr

parkr Jan 4, 2016

Member

you can make this just _, I think?

This comment has been minimized.

@envygeeks

envygeeks Jan 4, 2016

Contributor

You can if you wish but he probably left it _site so the API is clear.

@envygeeks

envygeeks Jan 4, 2016

Contributor

You can if you wish but he probably left it _site so the API is clear.

This comment has been minimized.

@pathawks

pathawks Jan 4, 2016

Member

he probably left it _site so the API is clear.

Yup

@pathawks

pathawks Jan 4, 2016

Member

he probably left it _site so the API is clear.

Yup

end
conflicting_urls
end
- def fsnotify_buggy?(site)
- return true if !Utils::Platforms.osx?
+ def fsnotify_buggy?(_site)

This comment has been minimized.

@parkr

parkr Jan 4, 2016

Member

why pass the site if it's not used?

@parkr

parkr Jan 4, 2016

Member

why pass the site if it's not used?

This comment has been minimized.

@pathawks

pathawks Jan 4, 2016

Member

Didn't want to do anything that would appear to change the API, since this is a public method.

I suppose it could be changed to def fsnotify_buggy?(_ = nil) and then not pass site?

@pathawks

pathawks Jan 4, 2016

Member

Didn't want to do anything that would appear to change the API, since this is a public method.

I suppose it could be changed to def fsnotify_buggy?(_ = nil) and then not pass site?

@@ -19,7 +18,7 @@ class Configuration < Hash
'safe' => false,
'include' => ['.htaccess'],
'exclude' => [],
- 'keep_files' => ['.git','.svn'],
+ 'keep_files' => ['.git', '.svn'],

This comment has been minimized.

@parkr

parkr Jan 4, 2016

Member

this could be the %w syntax

@parkr

parkr Jan 4, 2016

Member

this could be the %w syntax

- def i_month; @obj.date.strftime("%-m"); end
- def short_month; @obj.date.strftime("%b"); end
- def short_year; @obj.date.strftime("%y"); end
- def y_day; @obj.date.strftime("%j"); end

This comment has been minimized.

@parkr

parkr Jan 4, 2016

Member

i really prefer this shorter syntax – what's the rule it violates?

@parkr

parkr Jan 4, 2016

Member

i really prefer this shorter syntax – what's the rule it violates?

This comment has been minimized.

@envygeeks

envygeeks Jan 4, 2016

Contributor

Using a semicolon to terminate a line.

def year() @obj.date.strftime("%Y") end
@envygeeks

envygeeks Jan 4, 2016

Contributor

Using a semicolon to terminate a line.

def year() @obj.date.strftime("%Y") end

This comment has been minimized.

This comment has been minimized.

@envygeeks

envygeeks Jan 4, 2016

Contributor

I'll disable this rule, I personally liked the way @parkr grouped the methods.

@envygeeks

envygeeks Jan 4, 2016

Contributor

I'll disable this rule, I personally liked the way @parkr grouped the methods.

@@ -93,7 +92,7 @@ def generate_url_from_drop(template)
# as well as the beginning "/" so we can enforce and ensure it.
def sanitize_url(str)
- "/" + str.gsub(/\/{2,}/, "/").gsub(%r!\.+\/|\A/+!, "")
+ "/" + str.gsub(/\/{2,}/, "/").gsub(/\.+\/|\A\/+/, "")

This comment has been minimized.

@parkr

parkr Jan 4, 2016

Member

is %r not allowed?

@parkr

parkr Jan 4, 2016

Member

is %r not allowed?

This comment has been minimized.

@envygeeks

envygeeks Jan 4, 2016

Contributor

Both %r!! and // are allowed.

@envygeeks

envygeeks Jan 4, 2016

Contributor

Both %r!! and // are allowed.

This comment has been minimized.

This comment has been minimized.

@envygeeks

envygeeks Jan 4, 2016

Contributor

If we kick out AllowInnerSlashes: true then it will go on and require you to do %r!! for clarity, sound fine?

@envygeeks

envygeeks Jan 4, 2016

Contributor

If we kick out AllowInnerSlashes: true then it will go on and require you to do %r!! for clarity, sound fine?

This comment has been minimized.

@parkr

parkr Jan 4, 2016

Member

👍

@parkr

parkr Jan 4, 2016

Member

👍

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Jan 4, 2016

Member

Thanks, @pathawks!

Member

parkr commented Jan 4, 2016

Thanks, @pathawks!

parkr added a commit that referenced this pull request Jan 4, 2016

@parkr parkr merged commit 5580972 into jekyll:master Jan 4, 2016

1 check passed

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

parkr added a commit that referenced this pull request Jan 4, 2016

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Jan 4, 2016

Member

Down to 204 offenses. Thanks so much, @pathawks!

Member

parkr commented Jan 4, 2016

Down to 204 offenses. Thanks so much, @pathawks!

@pathawks pathawks deleted the pathawks:rubocop branch Jan 4, 2016

@pathawks

This comment has been minimized.

Show comment
Hide comment
@pathawks

pathawks Jan 4, 2016

Member

Thank you both for helping me through this. I learned a lot along the way 👍

Member

pathawks commented Jan 4, 2016

Thank you both for helping me through this. I learned a lot along the way 👍

@parkr parkr referenced this pull request Jun 15, 2016

Closed

Make jekyll/jekyll code compliant with rubocop rules #4885

114 of 115 tasks complete

@jekyll jekyll locked and limited conversation to collaborators Feb 27, 2017

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.