Hound: Review JavaScript, CoffeeScript, and Ruby code for style guide violations | Hacker News Reader

Hound: Review JavaScript, CoffeeScript, and Ruby code for style guide violations

houndci.com

67 pointsCoffeeOnWrite28 comments

I loves these sorts of robots because I hate being "that guy" who nitpicks over things like trailing white space. (It matters!!)

There's really no way not to sound like a pedantic jerk when you comment on someone's pull request about such things.

This at least makes the robot out to be the pedant.

the other thing to do is just go through the code base later, fixing those kinds of things. i.e., let it go on the pull request, but go through all the code and fix every instance once a month or something.

a few years ago, IIRC, Josh Peek did that for the Rails code base and that one commit got like five thousand "thank you" comments.

A problem with this is it adds useless commit history. If I make a significant change to some code, but then later on you remove some bad formatting/white spaces the latest commit messages for that code block is most likely "removed whitespace" and not something more meaningful.

That, and tasks that are left to be dealt with later are tasks that never get done.

99% of the time it actually doesn't matter, so don't comment on it.

Style guide violations can introduce bugs when moving from one system to another. At best, they slow down communication when someone can't easily understand your code.

Are you telling me someone can't understand my code because I left two spaces at the end of the line? Then they should be fired.

So I dug deeper and looked at Thoughtbot's guidelines [1] and found an epic guideline for Javascript:

> Use CoffeeScript

[1] https://github.com/thoughtbot/guides/blob/master/best-practi...

I like the idea behind this because I am also very sensitive to that type of things. Probably over-sensitive but I just worry that if you let one thing go, it becomes a slippery slope and you then have a mess of a repo.

That being said, if you care about these things, I wonder if these checks are best left to a pre-commit hook. It removes noise from the commit history and the PRs and forces people to think about it right away, rather than being corrected after the fact.

I guess having that done on a server has the benefit of not having to worry about keeping the linters' version up-to-date/homogenous over all the devs' machines.

At work, we have JSCS (https://github.com/jscs-dev/node-jscs) and SCSS-lint (https://github.com/causes/scss-lint) as part of our pre-commit hook (on top of editor plugins) and that has been great honestly. It decreased the PR noise a lot and I feel it has been good for new hires since it avoids having the first PR comments being about style issues.

I wrote a post about how I added JSCS in the pre-commit hook by the way: http://tech.adroll.com/blog/web/2014/03/05/adding-jscs-to-yo... It should be easily extendable to other linters.

This question comes up often when I tell people about my project Landscape (https://landscape.io), which is similar to this except that it is for Python.

My argument is that often, especially on a large existing codebase, you'll get thousands of warnings and in that case, having the trends over time is useful as a way of measuring progress. The relative change is more important than the absolute value.

We use a linter (jshintrc) and we put a .jshintrc with the configs we agree on our team on every project.

We work a lot with opensource projects and you need to respect the project style. Since some of our stuff was created from other things, we can't even keep the same jshintrc configuration for all our things.

I use SublimeLinter [1] in Sublime. It shows me the errors in an integrated fashion.

Once the project has some maturity it is better to not change the rules because you don't want to waste a lot of time changing stupid things like double quotes to single quotes.

If I see 150 errors in a file, it is likely that I will introduce the 151, and it usually means that your jshintrc is a fantasy. If I see less than 5 errors I will fix them all and use git blame to know who introduce these bugs and kindly recommend using an integrated linter with the editor.

I don't think is good idea to run a linter like this on any kind of CI. If someone breaks something serious the test suite will fail anyway, and if is not so serious why wasting time fixing it. I think is definitely better an easier to educate your team to use some plugin on their editor.

[1]: https://github.com/SublimeLinter/SublimeLinter-for-ST2

Those tools could just fix style, instead of just whining about it.

I'm not even sure why code editor can produce code that does not obey agreed upon stylistic rules. Oh, wait. It's because it's not a code editor, just a text editor. Why don't we have cod editors yet?

Regarding your first point:

Most good linters enforce as many "best practices" as they enforce "style suggestions".

Imho: It's important you know why / what they recommend.

Regarding the second:

100% Agree.

> Why don't we have cod editors yet?

I was going to snark that we do, they're called IDEs and they're horrible, but then I realized something.

In between an IDE and a text editor, there could totally exist a bona fide code editor, an in-between kind of system, but we (devs) tend to cluster our solutions under two categories, namely text editors (raw, simple, direct) and IDEs (complex, featureful, enormous).

This reminds me of a weird thing about testing frameworks - staggering numbers of testing frameworks get written, yet nearly all of them tend to either use "assert"-style syntax or "should/expect"-style syntax.

There could totally exist other families of syntax, but there don't. (Actually, there probably do, but if so, I'm not aware of them, and it could be because they're far from the mainstream.)

So with both these examples, you have this incredible diversity of implementations which can be divided really easily into just two basic categories. So even though there's an enormous amount of diversity at a very granular level, just one conceptual level up, there's almost no diversity at all.

So because of all this I have to say that I think your question is actually a much better question than I initially thought.

I remember when Hound first launched and every time somebody commented on the same line of code, Hound would persistently comment back with the same warning. It was amusing!

I'm currently working on a PHP version of Hound, called Anorak[1]. At the moment I'm part of a small team building a PHP replacement of Rubocop called Hippo[2]. We originally wrote a basic tokenizer with regex, but although it works well, we realise it's slow, so we're rebuilding it with "token_get_all" and hacking around that.

[1] http://anorakci.com

[2] https://github.com/hippophp/hippo

Really helpful and useful tool but we ultimately had to quit using it because of the noise. Rather than a comment per violation I'd like to see an approach similar to CodeClimate's: one comment and a link to a more detailed report.

It's interesting that Hound itself is a Rails app [1]. And it seems to be really well organized / put together. Thoughtbot is definitely following Sandi Metz's rules for developers [2].

There are a lot of small, plain Ruby classes that are very easy to understand. Must be relatively easy to maintain and add new features.

[1] https://github.com/thoughtbot/hound

[2] http://robots.thoughtbot.com/sandi-metz-rules-for-developers

I've been using jscs [1] for js style checks.

[1] https://github.com/jscs-dev/node-jscs

JSCS is quite good. Also, I use the jsdoc plugin to validate and make sure the jsdoc comments are up-to date.

For people whinging about "why doesn't it just fix the small stuff", I've been using Google's Closure Linter for years to fix JS style with the `fixjsstyle` command. It's grand for that purpose:

https://developers.google.com/closure/utilities/docs/linter_...

We use linters as part of our CI process. So actually "tests" fail if code is not within standards of rubocop or coffee lint.

Personally I prefer this over cluttering the PR process.

Codelinters are imho less about "style" than about best practices. And many exist for good reason other just for uniformity. Both is usually important enough to enforce it.

The point of contention on this is if you are doing something approximating continuous deployment. In that scenario, the tests and build are part of operations, since they are prerequisites to being able to ship code. As such, in an emergency scenario, you do not want to be in a situation where time to recovery is impacted by someone committing a hotfix that has improper whitespace, etc. In the best case, you end up spending additional time having to to 'sign off' on the broken build being just due to the linter (which also introduces a real risk in a high stakes scenario, in that you are shipping a red build and now have another place for human error to creep in) or in the worst case, the system blocks deploys until a commit is pushed to undo the whitespace problem.

1) real emergencies should happen very rarely

2) if they do you dont want to wait until the tests pass - so it's outside of that process anyhow

3) the real root problem here is: "emergency deploys need to be fast and adhoc" - not "linting is part of the ci process"

I'm not sure I agree -- having an alternate process for deployments in emergency scenarios is just asking for all kinds of trouble. It should be a well oiled machine that is consistent. If running the tests is part of the process, it should always be part of the process, otherwise you introduce additional complexity in trying to understand the potential outcomes of the system in unique scenarios. If the tests are slow that they gunk up the works, then the tests need to be fixed and the process changed. (Perhaps a large suite which runs nightly over less critical code, and a tight suite for mission critical code that runs pre-deploy.)

We do too, and it is tied into the TDD cycle.

Guard will first execute the rspec tests, and only if those pass it will execute rubocop. This way you don't have rubocop immediatelly complaning if you make a style violation and can focus on making the code work first. Once you are done with that you will still nagged about style before you commit your code.

Yea we have it the other way round.

Usually tests take quite a bit and you dont want to wait forever to finally see an obvious style violation.

Our tests take < 10secs and after implementing a commit worth of changes (I would say ours are comparatively small) there are usually less than 3 style violations piled up.

This looks pretty cool as a super easy to configure CI! I'm sure big projects will stick to Travis, but if you haven't got your linter set up yet this looks ideal.

I wonder how hard it would be for the bot to autofix stuff too? Perhaps it could serve a git remote and allow you to pull all the fixes in one go?

Shame this only works with GitHub. We use stash at work. And I would LOVE to have this. I'm very sensitive to this stuff, and I always have to be that guy :/

Reply on news.ycombinator.com