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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rubocop: test/* #4947

Merged
merged 12 commits into from May 26, 2016
Merged

Rubocop: test/* #4947

merged 12 commits into from May 26, 2016

Conversation

pathawks
Copy link
Member

@pathawks pathawks commented May 25, 2016

"markdown" => "kramdown",
:permalink => "date",
"baseurl" => "/",
:include => [".htaccess"],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be %w(.htaccess), did it not throw an error for that?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No sir, it did not.

@pathawks
Copy link
Member Author

✅ Rubocop Approves

@@ -1,23 +1,25 @@
require 'helper'
# rubocop:disable Metrics/ClassLength
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we should just disable Metrics/ClassLength for all of test/?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only way to do that would be to exclude all of tests from the metrics (I'm not against this, I wonder what @parkr thinks?) Other than that there is no way other than by file.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe we can disable just this metric from .rubocop.yml

If you don’t mind, I’ll give it a shot

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I say go for it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pathawks Yep, you can safely exclude all of test/ from Metrics/ClassLength. I still would like them tested for the other cops as they're arguably the worst part of our codebase 😆

@envygeeks
Copy link
Contributor

:shipit:

@parkr parkr added the pending-feedback We are waiting for more info. label May 25, 2016
@pathawks
Copy link
Member Author

These merge conflicts are killing me 😭

@jekyllbot jekyllbot removed the pending-feedback We are waiting for more info. label May 26, 2016
@envygeeks
Copy link
Contributor

haha, yeah that'll happen when you send a million pull requests :P

@@ -68,6 +57,7 @@ Metrics/ClassLength:
Max: 240
Exclude:
- !ruby/regexp /features\/.*.rb$/
- !ruby/regexp /test\/.*.rb$/
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, you can safely exclude all of test/ from Metrics/ClassLength.

You got it, boss 👍

@parkr
Copy link
Member

parkr commented May 26, 2016

@pathawks ❤️ ❤️ ❤️ ❤️ ❤️ ❤️ ❤️ ❤️

@jekyllbot: merge +dev

@jekyllbot jekyllbot merged commit cd63272 into jekyll:master May 26, 2016
jekyllbot added a commit that referenced this pull request May 26, 2016
@pathawks pathawks deleted the rubocop/tests branch May 26, 2016 16:52
@jekyll jekyll locked and limited conversation to collaborators Jul 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants