Trying to appease Rubocop #4301

pathawks · 2015-12-30T18:57:46Z

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 · 2015-12-30T19:07:38Z

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 · 2015-12-30T19:07:56Z

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

pathawks · 2015-12-30T19:09:17Z

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

pathawks · 2015-12-30T19:25:09Z

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

pathawks · 2015-12-30T22:53:39Z

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 · 2015-12-30T22:55:25Z

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 · 2015-12-31T20:38:21Z

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)


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

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.

Do you have a suggestion for making it more clear?

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

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

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

pathawks · 2016-01-01T19:53:03Z

Before:

119 files inspected, 1853 offenses detected

After:

119 files inspected, 1066 offenses detected

pathawks · 2016-01-03T08:46:33Z

master after b7d5a26:

69 files inspected, 606 offenses detected

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

envygeeks · 2016-01-03T22:09:15Z

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

- Use array literal [] instead of Array.new - Use hash literal {} instead of Hash.new

- Do not use semicolons to terminate expressions

- Use \ instead of + or << to concatenate those strings

- Prefer $LOAD_PATH over $:

- Use && instead of and - Use || instead of or

- %w-literals should be delimited by ( and ) Rubocop: Style/WordArray - Use %w or %W for array of words

- Hash#has_key? is deprecated in favor of Hash#key? Add method `key?` to Drop

- Use delete! instead of gsub! - Use tr instead of gsub

- Use hash rockets syntax

- Extra empty line detected at class body end

- Space missing after comma

Rubocop: Style/SpaceInsideBlockBraces

- Favor unless over if for negative conditions

- 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 · 2016-01-04T00:13:28Z

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 · 2016-01-04T00:15:44Z

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.

- Use proc instead of Proc.new ...and use lambda instead of proc

- Unused block argument

parkr · 2016-01-04T00:59:24Z

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

- Use next to skip iteration

- Indent when as deep as case

- Align else with if Rubocop: Lint/EndAlignment - Align end with if

- Use `attr_writer` to define trivial writer methods

- Ternary operators must not be nested. Prefer if/else constructs instead.

- Rewrite these with the positive case first

- Avoid the use of Perl-style backrefs

- Avoid using {...} for multi-line blocks

- Space inside range literal

- Don't use parentheses around the condition of an if

- Trailing whitespace detected Rubocop: Style/EmptyLines - Extra blank line detected Rubocop: Style/EmptyLinesAroundBlockBody - Extra empty line detected at block body beginning

parkr · 2016-01-04T20:24:01Z

lib/jekyll/commands/build.rb

@@ -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)


you can make this just _, I think?

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

he probably left it _site so the API is clear.

Yup

parkr · 2016-01-04T20:26:21Z

Thanks, @pathawks!

Merge pull request 4301

[ci skip]

parkr · 2016-01-04T20:34:19Z

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

pathawks · 2016-01-04T20:37:36Z

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

pathawks reviewed Dec 31, 2015
View reviewed changes

pathawks added 15 commits January 3, 2016 14:39

Rubocop: Style/EmptyLiteral

31dd0eb

- Use array literal [] instead of Array.new - Use hash literal {} instead of Hash.new

Rubocop: Style/Semicolon

44d2995

- Do not use semicolons to terminate expressions

Rubocop: Style/LineEndConcatenation

0eae36a

- Use \ instead of + or << to concatenate those strings

Use select and find instead of conditional loop

8223ebd

Rubocop: Style/SpecialGlobalVars

6550867

- Prefer $LOAD_PATH over $:

Rubocop: Style/AndOr

fb0457b

- Use && instead of and - Use || instead of or

Rubocop: Style/PercentLiteralDelimiters

98a19cd

- %w-literals should be delimited by ( and ) Rubocop: Style/WordArray - Use %w or %W for array of words

Rubocop: Style/DeprecatedHashMethods

ff5f7b7

- Hash#has_key? is deprecated in favor of Hash#key? Add method `key?` to Drop

Rubocop: Performance/StringReplacement

d157a04

- Use delete! instead of gsub! - Use tr instead of gsub

Rubocop: Style/HashSyntax

2530a8c

- Use hash rockets syntax

Rubocop: Style/EmptyLinesAroundClassBody

cda226d

- Extra empty line detected at class body end

Rubocop: Style/SpaceAfterComma

a70d89a

- Space missing after comma

Rubocop: Style/SpaceBeforeBlockBraces

663a2d3

Rubocop: Style/SpaceInsideBlockBraces

Rubocop: Style/NegatedIf

704ca6b

- Favor unless over if for negative conditions

Rubocop: Style/SymbolProc

af5d512

- 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 added 2 commits January 3, 2016 16:18

Rubocop: Style/Proc

7ca4f7c

- Use proc instead of Proc.new ...and use lambda instead of proc

Rubocop: Lint/UnusedBlockArgument

11f0aab

- Unused block argument

pathawks added 15 commits January 4, 2016 11:10

Rubocop: Style/Next

2c9a349

- Use next to skip iteration

Rubocop: Style/CaseIndentation

f6fd901

- Indent when as deep as case

Rubocop: Style/ElseAlignment

af9ec68

- Align else with if Rubocop: Lint/EndAlignment - Align end with if

Rubocop: Style/TrivialAccessors

f9926ed

- Use `attr_writer` to define trivial writer methods

Rubocop: Style/IndentationWidth

78e9f33

Rubocop: Style/NestedTernaryOperator

085a778

- Ternary operators must not be nested. Prefer if/else constructs instead.

Rubocop: Lint/UselessAssignment

ec83ef6

Rubocop: Do not use unless with else

be3666f

- Rewrite these with the positive case first

Rubocop: Style/PerlBackrefs

086e85c

- Avoid the use of Perl-style backrefs

Rubocop: Style/BlockDelimiters

6711234

- Avoid using {...} for multi-line blocks

Rubocop: Style/SpaceInsideRangeLiteral

04e635b

- Space inside range literal

Rubocop: Style/SpaceInsideHashLiteralBraces

c1c8b6d

Rubocop: Avoid single-line method definitions

cce848d

Rubocop: Style/ParenthesesAroundCondition

ab3d906

- Don't use parentheses around the condition of an if

Rubocop: Style/TrailingWhitespace

060904d

- Trailing whitespace detected Rubocop: Style/EmptyLines - Extra blank line detected Rubocop: Style/EmptyLinesAroundBlockBody - Extra empty line detected at block body beginning

parkr reviewed Jan 4, 2016
View reviewed changes

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

Merge pull request #4301 from pathawks/rubocop

5580972

Merge pull request 4301

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

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

Update history to reflect merge of #4301

eaade1e

[ci skip]

pathawks deleted the rubocop branch January 4, 2016 20:36

parkr mentioned this pull request Jun 15, 2016

Make jekyll/jekyll code compliant with rubocop rules #4885

Closed

jekyll locked and limited conversation to collaborators Feb 27, 2017

jekyllbot added the frozen-due-to-age label Feb 27, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Trying to appease Rubocop #4301

Trying to appease Rubocop #4301

pathawks commented Dec 30, 2015

envygeeks commented Dec 30, 2015

envygeeks commented Dec 30, 2015

pathawks commented Dec 30, 2015

pathawks commented Dec 30, 2015

pathawks commented Dec 30, 2015

envygeeks commented Dec 30, 2015

pathawks Dec 31, 2015

envygeeks Dec 31, 2015

pathawks Dec 31, 2015

envygeeks Dec 31, 2015

envygeeks Dec 31, 2015

pathawks Dec 31, 2015

pathawks commented Jan 1, 2016

pathawks commented Jan 3, 2016

envygeeks commented Jan 3, 2016

pathawks commented Jan 4, 2016

envygeeks commented Jan 4, 2016

parkr commented Jan 4, 2016

parkr Jan 4, 2016

envygeeks Jan 4, 2016

pathawks Jan 4, 2016

parkr commented Jan 4, 2016

parkr commented Jan 4, 2016

pathawks commented Jan 4, 2016

Trying to appease Rubocop #4301

Trying to appease Rubocop #4301

Conversation

pathawks commented Dec 30, 2015

envygeeks commented Dec 30, 2015

envygeeks commented Dec 30, 2015

pathawks commented Dec 30, 2015

pathawks commented Dec 30, 2015

pathawks commented Dec 30, 2015

envygeeks commented Dec 30, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pathawks commented Jan 1, 2016

pathawks commented Jan 3, 2016

envygeeks commented Jan 3, 2016

pathawks commented Jan 4, 2016

envygeeks commented Jan 4, 2016

parkr commented Jan 4, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parkr commented Jan 4, 2016

parkr commented Jan 4, 2016

pathawks commented Jan 4, 2016