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 #162

Closed
wants to merge 2 commits into from
Closed

Rubocop #162

wants to merge 2 commits into from

Conversation

maschwenk
Copy link
Contributor

@maschwenk maschwenk commented Jan 4, 2018

In working on #161 I've noticed this repo's style is quite specific. I was wondering if you guys would consider standardizing the style you guys use through a Rubocop config?

This initial commit just makes it so that all Rubocop rules pass, I wanted to put this here as a PR before I actually went ahead and actually made some suggestions on corrections.

Basically, I'd just look at the cases where there are just 1 or 2 occurrences of the violation and fixing them. If there are instances where you guys seem to be making an intentional choice of personal style, for instance:

# Offense count: 52
# Cop supports --auto-correct.
# Configuration parameters: AllowMultipleReturnValues.
Style/RedundantReturn:
  Enabled: false

e.g.

    def app_id(app_id, user_id, options = {})
      .
      .
      return secret
    end

That'd be something I'd leave alone.

Let me know your thoughts, totally understand if it's not to your preference. I find having a Rubocop config makes it easier for outside contributors to not spend a bunch of time wondering if their PR will fit into your style guide.

@evanphx
Copy link
Contributor

evanphx commented Feb 27, 2018

Rubocop has so many things that I disagree with I don't really want to constantly maintain an exclusions list. If we were to use it, I'd want a global exclusions list that applies to all files, no having to set them per file.

@maschwenk
Copy link
Contributor Author

Understood

@maschwenk maschwenk closed this Feb 27, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants