swipely/docker-api — pull request #224 by mrflip (2014-12-02 03:22:40+00:00)
Title: Inflexible line length limits considered harmful

Limiting line lengths to 80 characters leads to bad code (I'll justify below). This patch

* Relaxes the line length limit to 160
* As a correctness test, changed something that is arguably better on one line than two
* Explains why cane and proposes a policy (which you should tell me what you want the actual policy to be)

Maybe someone on your side is really insistent on this. If one of the code owners has a "but when I see that I itch and just need to fix it", well, I know how that is. Close this as /wontfix/ and the 80-character limit shall stand. Also, Linus says it's a thing to do, and that should carry some weight.

But if it's only in place because cane is a jerk by default (that's it's job, it's in the name; but we're wise as well as just) then it should be relaxed. This patch doubles the limit to 160 characters -- those still coding on Z-80 terminals can just set two side-by-side, and at 160 characters yeah maybe two lines are called for. It's clear that you guys do a very good job with code review, and especially on calling out style inconsistencies. We're all thoughful professionals and if essential information is hanging out at column 120 you'll catch it and we'll fix it.

I'm mindful that walking in to somebody else's project and criticizing their style guide is bad form. But I found myself writing up what follows; I'm supposed to blog more and I guess it struck a nerve. Since maybe you'll find it persuasive or didn't care in the first place, here is this pull request anyway. If it is unwelcome crankery, however, please forgive, because it is at the very least _thoughtful_ unwelcome crankery.

_____

A rule I'd stand behind is to put all essential code within the first 80 characters; with good taste, extend details that are both uncomplicated and inessential into the margins. This lets you set a visual rhythm in which the weight of the code on the screen matches the weight of attention you should allot it.

Bad outcomes of a robot taking those margins away:

1. **Coding as if looking over my shoulder is a headmaster with a literal cane**. Multiple times while preparing patches, I found myself counting characters up to 80. Not what one should be focused on. It also felt pretty lame when I pushed the first patch after checking that it passed rake spec (oops not rake) and then found out Travis was failing the patch for really? That?

2. **Clumsiness or even worse cleverness**. I wanted to change this:

```
# 80 chars indented
"Docker::Container { :id => #{self.id}, :connection => #{self.connection} }"
```

to this:

```
# 81 chars indented
"#{self.class.name) { :id => #{self.id}, :connection => #{self.connection} }"
```

I chose to solve it by being clever, which is wrong, and you caught me out:

```
# 80 chars indented
self.class.name+" { :id => #{self.id}, :connection => #{self.connection} }"
```

So we're left with this clumsy thing:

```
# a backslash for someone to put a space next to, and two lines of my attention
self.class.name+" { :id => #{self.id}, "\
":connection => #{self.connection} }"
```

A `to_s` is not pretty code, and it's not interesting code. It's a necessary detail and once I see the method name that typically tells me everything I need to know about it.

3. **Uninformative error messages**. There is at least one place [place](https://github.com/swipely/docker-api/blob/master/lib/docker/image.rb#L29) (75 chars) where I suspect the message got less clear because someone had to take a few beats (see solecism 1) and reword it to make weight. It's always best to put the value of an ill-formed parameter in the raise -- `raise "can't recognize ip_address: '#{ip_address}'"` is much easier to debug when you see an empty string or a domain name in there -- and this discourages that.

4. **Non-rubyisms**. Ruby gives you syntactical tools that encourage you to write code as you write for people. You can vary tone: brisk (`select(:&good?)`), conversational (`select{|obj| ...simple checks of goodness...}`), deliberate (`select do |obj| ...weighty multiline determination of goodness.. end`). The line length limits forces everything uniformly in tone to the weighty version. Ruby allows you to match sentence structure to meaning:

- This means "pay attention to what we capture in the regexp; we'll also take this opportunity to reject an obvious edge case":

```
from_rt_parts = /\A(.+):(.+)\z/.match(from_repo_tag) or raise Docker::Error::ArgumentError, "Need a full source repo:tag name: '#{from_repo_tag}'"
into_rt_parts = /\A(.+):(.+)\z/.match(into_repo_tag) or raise Docker::Error::ArgumentError, "Need a full target repo:tag name: '#{into_repo_tag}'"
```

- This means "Taking care of an obvious but important edge cases, then we can get cracking". The conditionals are inessential, and the parallel construction of the exceptions and conditions makes it easy to read:

```
raise Docker::Error::ArgumentError, "Supply a full repo:tag name: '#{repo_tag}'" unless repo_tag =~ /\A(.+):(.+)\z/
raise Docker::Error::ArgumentError, "Container count must be within 1..10: '#{ctr_count}'" unless (1..10).include?(ctr_count.to_i)
raise Docker::Error::UnauthorizedError, "No credentials given, call Docker.authenticate! first" unless creds
...
```

- This means "I want you to know that these are special cases, that we have unusually stringent or differing requirements here":

```
unless repo_tag =~ /\A(.+):(.+)\z/
raise Docker::Error::ArgumentError, "Supply a full repo:tag name: '#{repo_tag}'"
end
if repo_tag =~ /\Aindex\.docker\.io/
raise Docker::Error::ArgumentError, "To push to the central docker repo, you must omit the 'index.docker.io' prefix: '#{repo_tag}'"
end
```

- This means "I chafe under the rule of a capricious unfeeling robot overlord":

```
unless repo_tag =~ /\A(.+):(.+)\z/
raise Docker::Error::ArgumentError, "Need full repo:tag name: #{repo_tag}"
end
unless (1..10).include?(ctr_count.to_i)
raise Docker::Error::ArgumentError, "Container count must be 1<= x <=10"
end
raise Docker::Error::UnauthorizedError, "No credentials given" unless creds
```

5. **Disrupts visual rhythm**. Those different forms put different pieces foremost, and they also bear different visual weight. These tell different stories:

- A:
> I walked into the house and there he was and omygosh I wanted
> to die but then he kissed me and everything was alright.

- B:
> I walked into the house, and there he was and
>
> omygosh.
>
> I wanted to die.
>
> But then he kissed me and
>
> everything
>
> was
>
> alright

- C:
> I walked into the house, and there he was and omygosh I wanted to die. But then
>
> he
> kissed
> me
>
> and everything was alright.

6. **Fewer comments**. This method assembles the hash docker wants from a configuration manifest object:

```
def self.raw_create_hsh(ctr)
pub_ports = ctr.ports.published_creation_hshs
exp_ports = ctr.ports.exposed_creation_hshs
vol_spec = {}
ctr.volumes.each{|vol| vol_spec[vol.gsub(/:.*/,'')] = Hash.new }
{
'name' => ctr.name,
'Image' => ctr.image_repo_tag,
'Entrypoint' => ctr.entrypoint,
'Cmd' => ctr.entry_args,
'Hostname' => ctr.hostname,
'Volumes' => vol_spec,
'HostConfig' =>{
'Links' => ctr.links.map(&:to_s), # 'container_name:alias'
'Binds' => ctr.volumes, # 'path', 'hpath:cpath', 'hpath:cpath:ro'
'VolumesFrom' => ctr.volumes_from, # ctr_name:ro or ctr_name:rw
'RestartPolicy' => {}, # or {'Name'=>'always'} or { 'Name' => 'on-failure', 'MaximumRetryCount' => count }
'ExposedPorts' => exp_ports, # { "/: {}" }
'PortBindings' => pub_ports, # { /: [{ "HostPort": "" }] } -- port is a string
'PublishAllPorts' => true,
}
}
end
```

There's no world where that works with an 80-character limit. In the worst-case scenario I waste time dirdling the comments to evade the Headmaster's attention.