rubocop: jekyll/lib/frontmatter_defaults.rb #4974

Merged
merged 2 commits into from Jun 2, 2016

Conversation

Projects
None yet
3 participants
@ayastreb
Contributor

ayastreb commented Jun 2, 2016

#4885

  • fixed code style
  • renamed has_precedence? to precedence?
lib/jekyll/frontmatter_defaults.rb
@@ -131,18 +136,18 @@ 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.

@envygeeks

envygeeks Jun 2, 2016

Contributor

@ayastreb this will have to be reverted back to it's original name, it's an API change that we cannot accept in a minor version. Feel free to "XXX" it for change in 4.0 but we might have people who somehow rely on it in some way shape or form, so we should keep it as is and tell Rubocop to silence it for now.

@envygeeks

envygeeks Jun 2, 2016

Contributor

@ayastreb this will have to be reverted back to it's original name, it's an API change that we cannot accept in a minor version. Feel free to "XXX" it for change in 4.0 but we might have people who somehow rely on it in some way shape or form, so we should keep it as is and tell Rubocop to silence it for now.

lib/jekyll/frontmatter_defaults.rb
if new_path.length != old_path.length
new_path.length >= old_path.length
- elsif new_scope.key? 'type'
+ elsif new_scope.key? "type"

This comment has been minimized.

@envygeeks

envygeeks Jun 2, 2016

Contributor

This isn't your fault but do you mind wrapping the argument in ()

@envygeeks

envygeeks Jun 2, 2016

Contributor

This isn't your fault but do you mind wrapping the argument in ()

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Jun 2, 2016

Contributor

Thanks! Just a few comments.

Contributor

envygeeks commented Jun 2, 2016

Thanks! Just a few comments.

@envygeeks envygeeks added the needs-work label Jun 2, 2016

@ayastreb

This comment has been minimized.

Show comment
Hide comment
@ayastreb

ayastreb Jun 2, 2016

Contributor

Hm.. localy all tests are green. But I think I saw once this test also failed for me:

TestSite#test_: configuring sites should have an array for plugins by default.  [/home/travis/build/jekyll/jekyll/test/test_site.rb:7]
Minitest::Assertion: --- expected
+++ actual
@@ -1 +1 @@
-["/home/travis/build/jekyll/jekyll/_plugins"]
+["/home/travis/build/jekyll/jekyll/test/source/_plugins"]

Seems like this test fails randomly sometimes.
Is it a known issue or something new?

Contributor

ayastreb commented Jun 2, 2016

Hm.. localy all tests are green. But I think I saw once this test also failed for me:

TestSite#test_: configuring sites should have an array for plugins by default.  [/home/travis/build/jekyll/jekyll/test/test_site.rb:7]
Minitest::Assertion: --- expected
+++ actual
@@ -1 +1 @@
-["/home/travis/build/jekyll/jekyll/_plugins"]
+["/home/travis/build/jekyll/jekyll/test/source/_plugins"]

Seems like this test fails randomly sometimes.
Is it a known issue or something new?

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Jun 2, 2016

Contributor

@ayastreb some of our tests are fragile that's why we have RSpec in the works, I've just got around to pushing up the stuff I've done for RSpec so everyone else can chip and continue the work. I should really get on that, I'll restart the test for you.

Contributor

envygeeks commented Jun 2, 2016

@ayastreb some of our tests are fragile that's why we have RSpec in the works, I've just got around to pushing up the stuff I've done for RSpec so everyone else can chip and continue the work. I should really get on that, I'll restart the test for you.

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Jun 2, 2016

Contributor

@jekyllbot: merge +dev

Contributor

envygeeks commented Jun 2, 2016

@jekyllbot: merge +dev

@jekyllbot jekyllbot merged commit dc4b91b into jekyll:master Jun 2, 2016

1 check passed

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

jekyllbot added a commit that referenced this pull request Jun 2, 2016

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Jun 2, 2016

Contributor

❤️

Contributor

envygeeks commented Jun 2, 2016

❤️

@parkr parkr referenced this pull request Jun 15, 2016

Closed

Make jekyll/jekyll code compliant with rubocop rules #4885

114 of 115 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment