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.

Sign up for GitHub

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

Jump to bottom

Update MethodLength to 40 #4173

Closed
wants to merge 11 commits into from
Closed

Update MethodLength to 40 #4173

wants to merge 11 commits into from

Conversation

maestromac
Copy link
Contributor

@maestromac maestromac commented Oct 1, 2019 •
edited
Loading

What type of PR is this? (check all applicable)

  • Misc

Description

  • Increase line limit threshold to 40 lines
  • Fixed lints introduced by that
  • Remove /spec from being excluded from CodeCliamte

Related Tickets & Documents

n/a

Mobile & Desktop Screenshots/Recordings (if there are UI changes)

n/a

Added to documentation?

  • no documentation needed

lightalloy
lightalloy reviewed Oct 1, 2019
View reviewed changes
Copy link
Contributor

@lightalloy lightalloy left a comment

Choose a reason for hiding this comment

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

I have run rubocop with this config, and I think we should add several exceptions to this rule, I mean to specify the files where this rule is violated.
I'm all for loosening the threshold but enabling it,

@weekly-digest weekly-digest bot mentioned this pull request Oct 7, 2019
Closed
@maestromac maestromac requested a review from a team October 7, 2019 14:21
@ghost ghost requested review from benhalpern and removed request for a team October 7, 2019 14:21
@maestromac maestromac removed the request for review from benhalpern October 7, 2019 14:22
@maestromac
Copy link
Contributor Author

I have fixed a few lint and added some eluded files @lightalloy

but removing /spec from being excluded introduced 222 lints 😒

@maestromac maestromac changed the title [WIP] Update MethodLength to 40 Update MethodLength to 40 Oct 9, 2019
@pr-triage pr-triage bot added the PR: unreviewed bot applied label for PR's with no review label Oct 9, 2019
@lightalloy
Copy link
Contributor

@maestromac I looked into these issues and I think that we should have different codeclimate code settings for the /spec folder if that's possible, like disabling or increasing allowed file length. If we decide to do that I would make it a part of another pr.

@maestromac
Copy link
Contributor Author

@lightalloy I don't think it's possible to use multiple code climate config files and I can't figure out how to only exclude specs from being checked by the "line-length" metric. I think either we endure the lint (there seems to be 11 line-length lints for specs) and fix what we can or go back to excluding all the spec files again.

you have any thought on this @rhymes ?

@weekly-digest weekly-digest bot mentioned this pull request Oct 14, 2019
Closed
@maestromac
Copy link
Contributor Author

I will revisit this

@maestromac maestromac closed this Oct 31, 2019
@maestromac maestromac deleted the mac/misc/update-cc branch October 31, 2019 18:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lightalloy lightalloy lightalloy left review comments

Assignees
No one assigned
Labels
PR: unreviewed bot applied label for PR's with no review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@maestromac @lightalloy