Adopt a code style guide

[bf4]

Issue by linduxed
Saturday Nov 02, 2013 at 15:11 GMT
Originally opened as #93

---

Since I've started to look at some of the PRs here (along with the actual code) it would be nice to have an officially adopted style guide.

This would allow someone reviewing the code to point to specific issues in the code and reference the guide as a set of rules for what can and can't go into the code base.

I'm proposing the use of thoughtbot's "guides" repository as a start, since it's of high quality and constantly refined.

[linduxed]

As an update to this, I'd recommend using HoundCI as an automated way of enforcing the style guide. It only complains about diffs, so it won't comment on the entire repo.

By default HoundCI uses the thoughtbot style guide, but it can be tweaked with a .hound.yml file placed in the root of the repo.

[bf4]

Are you sure this won't cause all PRs to be spammed by houndi? Last time I used it, it was pretty noisy, and this codebase is very non-standard.

[linduxed]

It will comment in every pull request on style transgressions, but only on changed lines.

While this means it won't complain about everything in an affected file, there is indeed a chance that the diffs coming in would contain multiple style transgressions. It's hard to say how many that would be, but we would most likely have at least one complaint for every PR.

[ioquatix]

While I think having a consistent style is useful (but not essential), given how little time everyone has and the needs of this project at present, I believe that this may initially be counterproductive. If someone submits a pull request and a few things aren't up to spec technically, yeah, we should sort it out, but style, I think we can leave that up to common sense for now.

[ioquatix]

As an addendum, obviously things which are horribly organised or laid out should be fixed... I don't think anyone needs to refer to a style guide, either merge the PR and fix the style issue or ask the contributor to make the change. However, I think going back to the PR on issues like whitespace is pretty much a waste of everyones time, faster just to merge and fix the issues.

[linduxed]

This is a fair point; most people are probably neither as adamant as I am about code style and structure. Hound would probably be overkill.

I still think that the adoption of a style guide would be helpful to at least have something to point towards if the look of some supplied code is egregious.

[ioquatix]

@linduxed I really appreciate your POV and I think it's great to have someone on board who will do as you are proposing. Let's work on it, but firstly, lets try to fix the major outstanding issues with the code and get a proper release out.

[bf4]

So, can we table this issue and maybe just add something like https://github.com/metricfu/metric_fu/blob/master/spec/quality_spec.rb

[vassilevsky]

Let us enable Hound when it will be impossible to work without it (when maintainers will be drowning in badly formatted PRs). But no sooner. It will be a loss if some brave contributor will go away after a comment left by Hound.

[mockdeep]

Personally, I'm more likely to contribute to a project that has a style guide and some CI built around it. However, I'm not crazy about the way HoundCI works. If at any point the project gets pretty thoroughly ratcheted down in terms of style, new pull requests from people unfamiliar with the guide will spam everybody with comment notifications. Being that they're just using Rubocop behind the scenes, it's not that hard to plug into Travis and have it fail the build, where then you can go and look at the list of violations instead.

[vassilevsky]

That's a good idea.

I'm afraid though that a stand-alone Rubocop will produce a lot of noise because existing source does not conform to the community guide. Offenses related only to a certain pull request will be lost in the long list.=

[mockdeep]

Actually, rubocop is configurable, too. What I've done in this pull request is disable all of the rules, and then we can enable them one by one as we clean up the code.

[vassilevsky]

@mockdeep changed my opinion on Hound. At the moment, it's the best tool for style checking. It doesn't interfere with the main build. It only comments on changes. I suggest that we should enable it.

[mockdeep]

@vassilevsky I guess I don't see the problem with it causing the build to fail when people introduce style guide violations. It prevents code from being merged that is not consistent with the team's standards. A couple of problems I see with hound:

• As mentioned above, it can be noisy if someone new comes to our project who is unfamiliar with the style guide. It will spam all of the people subscribed to the project, as well as the person who needs to fix the issues. Even people who are familiar with the code will make noisy mistakes from time to time. This makes it more prone to become one of those annoyances you ignore more than listening to, and I could imagine it being very discouraging to newcomers.
• It isn't enforced mechanically. If we don't prevent inconsistent code from being merged, we end up with undocumented inconsistencies in our codebase, so when I run rubocop locally I can't be sure what violations I should care about.
• It puts an emphasis on fixing pre-existing style violations only in the code you're editing, which introduces more style inconsistencies until all of the code has been cleaned up. To me, style fixes are a separate concern from features and bug fixes, and should be pull requested separately for easier review.
• HoundCI doesn't support all of rubocop's rules, yet. Add to that, we're pinned to Hound's version of Rubocop, so we'll have to wait for them to update before we can use the latest and greatest from Rubocop, which has been a problem in the past.
• There's no way to run HoundCI locally, so we'll need to use Rubocop instead. This means having a .hound.yml file with our configurations and a .rubocop.yml that references it.

It's not a huge deal, overall, I guess, but I prefer a stricter approach where the control is in our hands.

[linduxed]

Considering Hound was recently (at least so it seems) turned on for this project, I'm thinking it is even more relevant to settle for some style guide and add a .hound.yml that reflects this.

Currently we've got a .rubocop.yml that was auto-generated with rubocop --auto-gen-config, which isn't exactly the best thing to do considering there's a strong possibility there was no explicit style that was followed in the old code.

[mockdeep]

The existing .rubocop.yml file has all of the rules disabled for the moment. The idea was that they could be discussed individually as they are enabled.

[vassilevsky]

Yes, I enabled Hound because why not :)

I had no idea Thoughbot prefers double quotes everywhere. I have now committed the .hound.yml file that points to .rubocop.yml.

I recently deleted a lot of rules from .rubocop.yml and ran rubocop -a after each deletion, and committed the changes. So, Ruby code in this repo isn't as bad as it was already :) I did not make controversial changes, just the most obvious ones.

I think that we need to continue deleting stuff from .rubocop.yml. Also, watch out for Hound comments that are wrong and fix something in the config so that they disappear or no longer added.

[mockdeep]

Yeah, I was going to try to get the test coverage up before jumping into rubocop completely, but most of it should be harmless. First thing is converting over to RSpec... not enough time...

[bf4]

Sounds good. If you want to introduce rspec just have it run with testunit, and occasionally move specs over :)

[mockdeep]

Yep, that's the plan.

[vassilevsky]

.rubocop.yml is in place.
Hound is checking PRs, using it.

I think this is no longer an issue.