Hacker News new | comments | show | ask | jobs | submit login
Hound: A service that comments on Ruby style violations in GitHub pull requests (thoughtbot.com)
69 points by wymy on Apr 9, 2014 | hide | past | web | favorite | 65 comments



Can someone shed some light on the reason why we're still bothering with "line is too long" in 2014?

I mean I'm not talking 300 characters-long lines here that's ridiculous, but the idea that a line with 90 characters is "too long" because some arbitrary limit in the width of certain terminal emulators back in the days was fixed at whatever number of columns seems really backwards.

So yeah, I'm open too any sane explanation of the why that is actually good practice beyond what I perceive as severe cargo culting.


Because you either have to decide on a number, or else implement automatic wrapping, and automatic wrapping can be bad for a number of reasons (which I won't get into here).

As long as you decide on a number, you want to pick something that is wide enough to be useful, but small enough that it doesn't lend itself to abuse (overly long variable names, 8 levels of indentation, and so on).

Also, it's nice to be able to look at two or three columns of code in a reasonable font size on your monitor, to view it in websites of normal width, and so on. And to know that you can still view code just fine on your 11" Macbook Air, even with a project-tree sidebar open in your editor.

For legibility, typeset columns are often recommended to contain something around 60 characters per line, although there's clearly a range. [1] Because code is often indented 1-4 levels (unlike normal text), it makes sense to expand this somewhat, and 80 is a nice round number for this.

So, 80 is probably the most common number I've seen for code line length, and there's absolutely nothing cargo-cult about it. It's simply that, if you don't want to implement line-wrapping, you need to pick something, and 80 is quite reasonable.

[1] http://en.wikipedia.org/wiki/Column_(typography)


Two reasons:

1: My 15" screen is about 160 characters at my preferred font size. When I want split screen to compare two files then each one should only be about 80 characters. There is even less space when using IDEs or text editors that include file drawers or other widgets.

2: Excessive characters on one line is a code smell just like excessive line counts in one method. You agree that 300 characters is too much. If 90 is too little then at what point should an automated tool trigger a warning?


Well, the problem really stems for what you call a "code smell".

All the good practices about programming will mention that:

1) Excessive line count in a method is a code smell 2) Excessive number of characters is also a code smell 3) Non descriptive variable names are universally considered bad practice.

Now my problem with all this dogma is that if you combine 1, 2 and 3, you're left with a severe conundrum. And to me, excessive number of lines in a method and naming your variables a,b,c,d,e are concessions that I prefer to avoid if I can simply crank up the number of characters on a line (taking into account indentation caused by conditionals, method definitions, blocks, etc).


(1) (2) (3) can all coexist :

First you fix (3), by naming variables in a nice, descriptive way - that possibly causes violations of (2).

Then you fix (2), by having lines/statements that are clear and don't do ten things at once - that possibly causes violations of (1).

And then, you fix (1), by refactoring your methods properly - and that is a valid goal by itself.

If instead you have longer lines, then the problem of your method being too big doesn't go away, it still does the same thing, you just don't notice it. If (1)+(2)+(3) would force you to split up your methods, then that's an indication that those methods should be split up anyway, even if you choose to ignore (2) and (3) - "too many lines of code" is a code smell simply because it correlates to "does too much", and even a one-line method can possibly be doing too much, if it's written in code-golf-style.


Back when I worked on a project with Thoughtbot I used to fight with them about this all the time :) 80 characters felt really restraining on anything but an 11inch MacBook Air. Limiting your ability to have descriptive names is a huge loss just to mindlessly follow some line convention from decades ago. I tend to like restraint, but when restraint is at odds with communicating to future developers I can't side with it. These days I try to keep lines 130 characters or less, likely just as random a number but at least more forgiving.


Whatever works for you, just configure Hound to comment on lines longer than 130 instead of 80.

https://gist.github.com/salbertson/d5eb2d42f68df9e18d10


It goes farther back than terminal emulators. the old IBM punchcards physically only had room for 80 characters.

I'm with you, though. 80 characters is really short, especially if you are indented more than a few stops. It also discourages descriptive variable/method names, which harms readability of code far more than slightly longer lines.


I like to indent two characters per level of indentation. If the indentation uses braces, I indent one for the brace and one more for the code.


Here's an example of how you could configure Hound to increase the line length limit.

https://gist.github.com/salbertson/d5eb2d42f68df9e18d10


80 characters is thoughtbot's style. You can always configure it to be some other number, or turn in off altogether.


500 years of typographical practice has shown our species that 80 characters is about as long as a line can be before it becomes difficult to read. To see this, take a novel from your bookshelf and count the characters per line.


Sure, but nothing says you can't shrink your text editor's window down to 80 characters wide if you like.


Are you talking about line wrapping? Because I don't know a single person who enjoys wrapped lines of code.


A lot of developers use vim, with split buffers. When code lines wrap, they look ugly.


But other text editors handle long lines perfectly well. I don't think other developers' choice of tools should dictate coding styles.

I exclude the tab/spaces debate from this because it has ramifications outside of mere stylistic concerns.


There are only two possible ways to deal with long lines extending past the edge of the window.

1. Scroll off the side of the window

Pain in the ass. Ever try diffing a file, and had to scroll?

2. Wrap

Also a pain in the ass. Formatting alignment, confusion over where important linebreaks are, etc.


3. Wrap, and use a good editor

Many editors don’t fully support soft wrapping, sadly. But if you’re lucky enough to use an editor that includes the necessary features, there are no problems with soft wrapping. Soft wrapping has the advantage that you never have to manually re-wrap a line. These editor features make soft wrapping useable:

Display noneditable symbols at wrap points to mark them as nonimportant linebreaks. In Vim, `set showbreak=->` to display “->” at wrapped lines. Emacs supports putting arrow symbols in the “fringe”. Visual Studio has a preference to enable this.

Indent wrapped lines at the same indentation as the start of the line, optionally plus extra indentation. Vim does not support this but there is a patch for Vim that makes this enable-able with `set breakindent`. Coda supports it fully, with customizable extra indentation. This is the least-supported, most-important feature of making wrapping useable.

Separate logical-line movement and visual-line movement. Pretty much every editor supports this. Logical-line movement is really only necessary in macros, anyway. In Vim, it’s j/k (or -/<Enter>) vs gj/gk. In Sublime Text, it’s up/down vs option-up/option-down (or some combination like that).


I've used everything you mentioned except the Emacs one.

They definitely help. I just find it inconvenient to need to scroll, and sometimes arrows don't fit my usage case..

Not the editor's fault in either instance, of course. :-)


vim also has tabs. I only use split buffers when I'm using my external monitor.


Using split window panes allows me to see more on one screen, without switching contexts (tabs).


In .vimrc:

    set nowrap


Yes, then things are hidden. Brilliant.


Only until you scroll.


We had to pick an arbitrary point, and 80 had enough history to make it common.

I definitely pass 80 characters when it makes sense, and I use the appropriate pragmas to make rubocop understand not to warn me. But for the most part, 80 characters as a limit helps me know when my lines are getting to long.


I have always liked the idea of these bots roaming github, looking for bugs to fix and code that could be improved. But, it almost always ends up annoying somehow. Either the change is a false positive, the bot doesn't understand the project structure, or the bot just spams changes. Software development has too many unwritten rules and social interactions for a bot to just send PRs and comments.

Perhaps another type of communication, other than issues, PRs, and comments, is needed just for bots. I would tolerate my projects being scanned, if I could separate the human and bot communication streams. (And, block bots when they don't work as expected.)


You can tune Hound (and Rubocop) to your liking:

https://houndci.com/configuration

> the change is a false positive

I've been using Hound on about a dozen projects the last few months and haven't seen false positives. We built it atop Rubocop, which has been pretty well-vetted.

> Software development has too many unwritten rules and social interactions for a bot

We've limited our guidelines to a subset that should almost always be "no argument, my bad, fixed in [SHA hash]." However, it comments instead of failing the build or mechanically changing the code because that the human should make the final decision about whether to make the change.


I love this. On just about any team I have worked on, whenever a fresh PR comes in everyone goes through it picking out style guide violations. It's tedious, annoying and sometimes arbitrary if the team doesn't have a solid style guide in place. Hound will let us get right to reviewing what matters: the code itself.

Up next: I would love a Unix utility I could filter my uncommitted diff through and get style guide violations before I even upload to GitHub. Use git for everything, baby.


This exists and is what Hound is built on. Rubocop runs lint checks against your Ruby code: https://github.com/bbatsov/rubocop/ . The Emacs integration is great; it highlights failures in place. Hound looks interesting, but having Emacs/Vim integration gives you a tighter feedback loop, so you don't have to wait until Pull Request time to discover these issues.


RuboCop is great, but one benefit of Hound is it only checks code that has changed. Retroactively enforcing code style is usually a bad idea.


You can also use linting tools as a build validation step (ours are invoked via things like `make myapp.pep8test`). These can also then be invoked yourself before making a commit. As jwinter says, this really tightens the loop when fixing trivial errors.

Almost all of the tools are very configurable in terms of which warnings/errors you want to ignore (e.g., let our max line length be 120 instead of 80), and give pretty good feedback (line numbers, exact error).

Another great benefit is that you have a neutral arbiter of what's the style for your codebase, which helps avoid ruffled feathers.


I figured such a tool must exist. Thank you for helping me remain lazy. I agree about having the tighter feedback loop.

I'm hoping to create a git filter that stops the commit from happening if there are style guide violations.


> filter my uncommitted diff through and get style guide violations before I even upload to GitHub

You should be able to do this using a pre-commit hook.


https://github.com/bbatsov/rubocop FTW. Integrate it into your test suite and you can see your mistakes before you push up a change for your team to see.


Rubocop is great! Hound is built on top of it.

We've tried putting style violations in the build and found there are enough edge cases that it isn't exactly the interaction we've wanted. Sometimes, we want the human to say "no, my pair and I broke the guideline on purpose and we're okay with it in this case." We don't want a broken build in those cases.

Totally agree with you about getting feedback earlier than opening a PR, though. Linters integrated into text editors are a great way to go.


There is also a guard plugin for immediate feedback: https://github.com/yujinakayama/guard-rubocop

Rubocop can even correct some style errors automatically now!


Hound is built on Rubocop


Take a look at https://github.com/mmozuras/pronto - does the same, has many adapters.


Hounds looks easier than pronto. Just turn it on for a repo and it does it's thing. With pronto looks like I have to configure it, then run commands (could make it part of default rake task, is suppose).


Yes, we run pronto when our Jenkins builds GitHub pull requests, so we get all those comments along with peer code reviews. And no separate web app is necessary.


One thing I've liked with hound is that when it comes up with what I'd consider a "false positive," such as a line with a url that can't be split, I can just reply inline to explain why that couldn't change. This pull has a lot of examples of that: https://github.com/thoughtbot/griddler/pull/119


It's hilarious how houndci acts like some stubborn human, responding to complaints with the exact same suggestion.


Yeah, that happens if you force push.


That's not good. I opened an issue on GitHub.

https://github.com/thoughtbot/hound/issues/182


The marketing site needs some work. There's no details about pricing that I can find and no information about configuring your own rules (I had to read the source code to see if that was possible). I am not going to sign up with GitHub to see either of those things, they need to be public.


It's a free service right now. We'll be taking all the suggestions and improving it as we go.


The announcement blog post implies otherwise:

> It places the focus value not on our super-secret-source-code but on our handling of the hosting, billing, and maintenance of that source code.


Sorry if it wasn't clear in the post but we do plan on charging for Hound in some way.

"We intend to charge in the near future to ensure Hound is sustainable, but public projects will continue to be free."


I don't expect this to be free. My point is that pricing should be clearly stated on the landing page for the product. You should have a pricing link or header, even if it just says "free for now, we're working on pricing".


Got it, thanks for the input.


This is really cool. Sure, similar tools have existed for a while, but Hound looks far simpler to setup and use than previous tools.

However, it would be nice to have this optionally run on every commit rather than only on pull requests. For my own personal projects, I obviously don't submit pull requests to myself, but I would like to have Hound double-check my code for me.


Not sure it should be obvious you don't. Pull requests are especially well-suited for a team environment, but using PRs in a solo project can still give a higher level of structure to the progression of your code. Gives you an opportunity to explain why you're doing what you're doing.


You can also accomplish this without pull requests by writing good messages in your merge commits like you would in a pull request description.


Agreed. This approach is especially nice for situations where you open up the code at a later point, so that people can see the historic progression in more ways than just the Git history. https://github.com/thoughtbot/hound/pull/1


This is great. Is it possible to use this as a rake task? I think it would be better to have team members run this locally just like they would run a test suite, make sure everything is up to par, and then open a PR. This would keep the communication stream in the PR nice and clean, focused only on code implementation.



This is a great product. I was working on a product when a new developer joined and started writing Ruby like he was writing in PHP, ignoring all of the programming language's idioms. What was worse was that with every commit where his code was "cleaned up", he'd put them back in!


Glad you like it!


Not sure I'd want to give this site Github access: http://filippo.io/Heartbleed/#houndci.com


Read the FAQ, this is expected when the server has been patched as long as the site is responding.


It was giving me a "houndci.com IS VULNERABLE." 45 mins ago.

http://l.ew.is/shared/houndci.com_IS_VULNERABLE.png


Is that still the case? I'm seeing a timeout when I run the test.

https://www.dropbox.com/s/txlzh5jzrt30afs/Screenshot%202014-...

Again, we have reissued our certificates and installed them since the servers were patched.


We re-signed our certificates after Heroku patched the issue.


I hate those github bots. Just use rubocop and run it in your CI.


Hound is opt-in, it won't ever add itself to your repo and start commenting.




Guidelines | FAQ | Support | API | Security | Lists | Bookmarklet | Legal | Apply to YC | Contact

Search: