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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rubocop version upgrade #7846

Merged
merged 12 commits into from Oct 16, 2019

Conversation

@donquxiote
Copy link
Contributor

donquxiote commented Oct 6, 2019

This is a 馃悰 bug fix.

Summary

Upgrades the rubocop dependency to 0.75.0, the latest version, so others who pull down the project can use the latest.

Also adds some inline disabling to resolve rubocop issues that appear to be intentionally written in for readability.

Verified all tests pass locally with script/cibuild

Context

Resolves #7836

donquxiote added 2 commits Oct 6, 2019
Fix RuboCop Performance offenses in test files (#7839)
@donquxiote donquxiote marked this pull request as ready for review Oct 6, 2019
@donquxiote

This comment has been minimized.

Copy link
Contributor Author

donquxiote commented Oct 7, 2019

using rubocop 0.75.0 triggers a few formatting warnings, which I've found and can add the rubcop:disable comment for as part of this PR, but I wasn't sure how liberal y'all like to be with those comments.

@ashmaroli

This comment has been minimized.

Copy link
Member

ashmaroli commented Oct 7, 2019

I've found and can add the rubcop:disable comment

@donquxiote We prefer having a .rubocop_todo.yml generated instead. I shall however leave it to you to figure out how to get the RuboCop-TODO file generated and what recommendations within it can be immediately tackled.
IMHO, there's no point in simply updating to the latest RuboCop version if we don't act on its recommendations with earnest.

@donquxiote

This comment has been minimized.

Copy link
Contributor Author

donquxiote commented Oct 14, 2019

After running rubocop with the auto-gen-config I've added the rubocop_todo that it generated. However using rubocop's inherit_from function in the config broke the rubocop check when I ran it locally. So I added the line from the rubocop_todo into the regular .rubocop.yml

@ashmaroli

This comment has been minimized.

Copy link
Member

ashmaroli commented Oct 15, 2019

So I added the line from the rubocop_todo into the regular .rubocop.yml

Since the only entry in the todo file is now excluded via the main config file, there is no point in having a todo file, is there..?

However, I see some whitespace changes as well. If those were made to appease the Layout/SpaceAroundOperators cop, then please revert those changes and disable the cop via the TODO file because it appears to have a bug. The cop is configured to AllowForAlignment by default. So it doesn't work as advertised.
Having it entered in the todo file will remind us to check again with a future RuboCop version.

Thank you.

@donquxiote

This comment has been minimized.

Copy link
Contributor Author

donquxiote commented Oct 16, 2019

You had a good point about the rubocop_todo file. That file is now present and the rubocop.yml should inherit from it. I also removed those spacing corrections and moved those to the todo file as well.

Copy link
Member

ashmaroli left a comment

LGTM! 馃憤

@ashmaroli

This comment has been minimized.

Copy link
Member

ashmaroli commented Oct 16, 2019

Thank you very much @donquxiote

@jekyllbot: merge +dev

@jekyllbot jekyllbot merged commit 22a9742 into jekyll:master Oct 16, 2019
7 checks passed
7 checks passed
SUITE: test / OS: ubuntu-latest
Details
SUITE: test / OS: windows-latest
Details
SUITE: default-site / OS: ubuntu-latest
Details
SUITE: default-site / OS: windows-latest
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
netlify/jekyllrb/deploy-preview Deploy preview ready!
Details
jekyllbot added a commit that referenced this pull request Oct 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can鈥檛 perform that action at this time.