Update Rubocop rules #4886

Merged
merged 8 commits into from May 12, 2016

Conversation

Projects
None yet
3 participants
@parkr
Member

parkr commented May 12, 2016

This merges the rule set from github/pages-gem .rubocop.yml with our set here, overriding what we have with what is present in pages-gem's Rubocop rules.

I sorted the keys so they're grouped a bit more logically and excluded all the files so we can enable one-by-one. It'd be an easy pull request to fix the rubocop errors and enable the file. 馃槃 Tracking in #4885.

@jekyll/core Will you please take a look? There are likely things we should tweak here.

parkr added some commits May 12, 2016

@parkr parkr referenced this pull request May 12, 2016

Closed

Make jekyll/jekyll code compliant with rubocop rules #4885

114 of 115 tasks complete
- - test/**/*
- - vendor/**/*
- - features/**/*
+ - lib/jekyll/cleaner.rb

This comment has been minimized.

@envygeeks

envygeeks May 12, 2016

Contributor

It seems this not only broadened our scope but possibly lessoned it no? This list would be mighty unmaintainable an seems refactor unfriendly. We shouldn't expect users to maintain our Rubocop file, my vote is for sticking with the old list at least on this part.

@envygeeks

envygeeks May 12, 2016

Contributor

It seems this not only broadened our scope but possibly lessoned it no? This list would be mighty unmaintainable an seems refactor unfriendly. We shouldn't expect users to maintain our Rubocop file, my vote is for sticking with the old list at least on this part.

This comment has been minimized.

@parkr

parkr May 12, 2016

Member

@envygeeks In terms of AllCops.Exclude, I added each file individually so we can go through each and enable one-by-one. It's easier to review PR's for one or a handful of files and it means we can add the CodeClimate CI status now instead of waiting.

@parkr

parkr May 12, 2016

Member

@envygeeks In terms of AllCops.Exclude, I added each file individually so we can go through each and enable one-by-one. It's easier to review PR's for one or a handful of files and it means we can add the CodeClimate CI status now instead of waiting.

.rubocop.yml
+ - vendor/**/*
+Lint/AmbiguousRegexpLiteral:
+ Exclude:
+ - features/step_definitions.rb

This comment has been minimized.

@envygeeks

envygeeks May 12, 2016

Contributor

You are actually working around a literal Ruby warning, so this error will still surface on other linters (like Atom Linter, Sublime Text Linter and others.) According to Cucumber themselves it seems the preferred way to fix this with them is to just use %r!! which works for all parties, it's less escaping!

@envygeeks

envygeeks May 12, 2016

Contributor

You are actually working around a literal Ruby warning, so this error will still surface on other linters (like Atom Linter, Sublime Text Linter and others.) According to Cucumber themselves it seems the preferred way to fix this with them is to just use %r!! which works for all parties, it's less escaping!

.rubocop.yml
+Style/SpaceInsideBrackets:
+ Enabled: false
+Style/StringLiterals:
+ Enabled: false

This comment has been minimized.

@envygeeks

envygeeks May 12, 2016

Contributor

I would love if we kicked this on, I like Github have a huge love of "" over '' because I find that I interpolate more than I don't but I'll digress that I don't even know if Github themselves enforces it in their own source.

@envygeeks

envygeeks May 12, 2016

Contributor

I would love if we kicked this on, I like Github have a huge love of "" over '' because I find that I interpolate more than I don't but I'll digress that I don't even know if Github themselves enforces it in their own source.

parkr added some commits May 12, 2016

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr May 12, 2016

Member

@jekyllbot: merge +dev

Member

parkr commented May 12, 2016

@jekyllbot: merge +dev

@jekyllbot jekyllbot merged commit 13eaa4f into master May 12, 2016

1 check passed

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

@jekyllbot jekyllbot deleted the enable-rubocop branch May 12, 2016

jekyllbot added a commit that referenced this pull request May 12, 2016

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