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

Hold on, I'll just whip up a quick batch of Rubocop fixes #1117

Merged
merged 13 commits into from
May 6, 2015

Conversation

jaredmoody
Copy link
Contributor

Two dashes of Style fixes, a pinch of Lint fixes, stir well and bake at #945

@PragTob
Copy link
Member

PragTob commented May 5, 2015

Hi there, thanks for those fixes :)

I wasn't sure about the rubocop_todo.yml but having an in repo list of styles and being able to catch regressions seems to be good enough to me. Also, that if we decide that we disagree with these configurations we could just move them over to our rubocop.yml

@jasonrclark - what do you think? :)

Tobi

@jaredmoody
Copy link
Contributor Author

The nice thing about having the .rubocop_todo.yml is that you can run rubocop right now and everything is green - you could even introduce rubocop to the CI build now.

You nailed it: going forward we can address the individual offenses by either removing them from the todo list so they run (and fixing them), or by moving the exception permanently into .rubocop.yml

I've been trying to address the offenses that seem less controversial first and put each cop in a new commit so you can discard any commits where there's a disagreement with the rubocop defaults.

Prior to having the todo file though, I was running a single cop at a time, which left me open to introducing regressions. I'm planning on continuing to knock these out so the todo file should shrink rapidly.

@jasonrclark
Copy link
Member

I'm in favor of the approach here. .rubocop_todo.yml is a nice way to communicate that we don't expect that list to live forever. Once it's gone and everything's green, we remove it and plug the main rubocop into CI (exciting!)

Might be worth a note over in #945 about how you're running things using this file, and the intended process, in case anyone else wants to help out. This would smooth the path for them as well.

Appreciate your diligence doing things in small batches and separate commits @jaredmoody. It's made it really easy to review and approve. ✨ 🚀 👟

@jaredmoody
Copy link
Contributor Author

Glad to help!

I can make a note in #945 on the todo file and the process once this is merged.

@jasonrclark
Copy link
Member

Only had the one comment that looks like you already addressed @jaredmoody. You set for a merge on this?

@jaredmoody
Copy link
Contributor Author

👍 👟 :shipit:

jasonrclark added a commit that referenced this pull request May 6, 2015
Hold on, I'll just whip up a quick batch of Rubocop fixes
@jasonrclark jasonrclark merged commit 95ec672 into shoes:master May 6, 2015
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.

3 participants