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

Adopt rubocop code style #47

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

cshaffer
Copy link
Contributor

Greetings! Big fan of brainstem. This PR introduces rubocop as a development dependency in hopes of maintaining a consistent code style going forward.

The .rubocop_todo.yml file kept track of all the offenses that rubocop couldn't automatically fix. If you would like to go this route, I would be willing to go through each pending offense and address accordingly.

No worries if rubocop isn't for you, in which case feel free to close.

@cantino
Copy link
Contributor

cantino commented Jan 30, 2015

Hey @cshaffer, thank you!

I'll have to go over some of these changes and think about which way we want to take them. I agree with many of them, although I don't think the " -> ' is worth it due to the amount of churn in the git blame log that would result. Also, from my reading, it has no practical speed implications.

@cshaffer
Copy link
Contributor Author

@cantino No problem. Preferring single quotes when there is no string interpolation needed is purely a style choice and does not provide any runtime benefits. I can disable that cop, and others if you'd prefer. Let me know.

@cshaffer
Copy link
Contributor Author

I've configured the string literal cop to prefer double quotes. How's this look @cantino?

@cantino
Copy link
Contributor

cantino commented Feb 17, 2015

I'm a fan of these changes. I'm currently working on a new version of Brainstem and would prefer to wait to do this change until after that merge, so that I don't get merge conflicts up the wazoo.

@cshaffer
Copy link
Contributor Author

Sounds like a good plan. Feel free to ping me after the fact, I'm happy to resolve merge conflicts later, or start over with the fresh code, whatever makes the most sense.

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

Successfully merging this pull request may close these issues.

2 participants