Code Review in CRuby

-We released a blog piece on the interview with Yukihiro “Matz” Matsumoto the other day. In the interview, we asked him about code review. Is there anything you want to add on to or disagree with his take on the subject matter?

This was an interesting topic to read. I remember him saying that he first questions whether he can do the maintenance work on the code he commits even if it is work or an open source project. I think what he said was on point. I totally agree with him. He briefly mentioned about having a firm determination on taking on somebody’s code. I’m reminded again that he is very good at verbalizing.

I assume, when he touched on code review, he was talking about mruby, which he’s been working on recently. The development process of mruby is a little different from CRuby’s.

CRuby doesn’t have the concept of code review such as a review structure in the first place. So, we directly commit code written by someone else to the Subversion trunk. There is no process or structure for someone to take a look at before committing. We first commit straight to the trunk since CRuby doesn’t have a set-up where you make a pull request and run the CI beforehand.

Then, there is RubyCI, which periodically checks the latest code out of the trunk, executes a test in the environment that Ruby supports, and displays the results. There are many matrices during this process, so some people would look at them.

Something close to a code review for CRuby would be PBmemo which is a blog published by a committer, nagachika. When the trunk breaks, he writes about it. Next day, the trunk is fixed.

PBmemo (available only in Japanese) http://d.hatena.ne.jp/nagachika/

-It makes me a little nervous to hear that the trunk breaks suddenly.

I think it is ok because only the trunk breaks. Issues will be solved by the time we release the code.

-On the other hand, mruby is reviewed on GitHub. Is this correct?

Yes, that’s right.

CRuby doesn’t have any particular flow to review the code, so there are many code that are committed to the trunk without getting reviewed. That’s why it often breaks. That is probably the most critical point of CRuby. I assume when Matz started working on a new Ruby project, he thought GitHub would be a good choice.

With mruby, you don’t directly push your code to the master branch. You need to create a pull request first, run the CI, review the code, then finally merge. I think mruby is small enough for Matz to handle everything with this process by himself. He probably learned from the current CRuby development system and improved the review process.

Coding Style and Coding Rules

Coding Style of CRuby

-How are you dealing with issues such as coding style in Ruby development?

Regarding guidelines for coding style of CRuby, just like I mentioned what Matz said in the interview earlier, it is ok to commit code in your own style as long as you understand the coding style of the original repository. But in actuality, I see both tab and space used for indentation. When you open any file you choose, I think you would see them coexisting in one place right away. Pick any C file and you will see that tab and space are used interchangeably. The shape of the code looks like a ria coast.

A picture of a typical ria coast. Photo by Serkan Turk on Unsplash

-I had a hard time reading the compile.c file. I could finally read it when I displayed with tab size at 8.

Excerpt of compile.c when displaying with tab size at 2. `if` is displayed on the left of `while’, which makes it hard to understand this code as the layout does not match the code structure.

When displaying with tab size at 8, the position of `if` within `while` matches the code structure.

My code editor displays up to tab size at 2, so the code will look jagged. I suggest vim users install a vim plugin, which is developed by Kenta Murata, to display the source code properly.

It is a little inconvenient that you need to make an extra step just for reading and writing the source code.

-By the way, which is used more between tab and space?

Space is more popular. But Nobuyoshi Nakada who is the most active CRuby committer prefers tab, so you see more indentation by tabs in the code.

Koichi Sasada uses tab. Nakada says he doesn’t care but I think he prefers tab. Akira Tanaka is the most active space advocate as far as I know. He commits after diligently changing from tab to space as the indentation of the line he fixed. Then, someone else edits the line and changes from space to tab. It is like a war between tab and space. If three-tab advocates come in, it will be chaotic. But, luckily, I think there is no three-tab user in CRuby.

This “tab or space” is a good example. We are not trying to unify the coding style to develop something.

Consequently, after creating a repository on GitHub and receiving pull requests, we start hearing from people that they don’t know which indentation style they should use. Back in the days, we never had this kind of discussion. But now, in the era of GitHub, I think it is better to create basic guidelines for the coding style.

And, that’s precisely what Ruby on Rails project is doing.

Coding Style of Rails

-In Rails, there is a guideline for the coding style and RuboCop checks code, right?

Yes, we have the machine do the automated checks at minimum level. We don’t use the default setting of RuboCop, but define our own minimal set of rules from scratch.

-What are the minimal rules?

For example, there is famous “DHH Indentation” which was added by DHH, the Rails project owner. With this rule, an extra indentation is inserted after private … It’s an awkward style. In IndentationConsistency = rails , as the name rails suggests, I’ve never seen anything like that other than Rails. Since the project owner insists, we use it…

There is one thing I personally don’t agree with. It is StringLiterals = double_quotes . It means all quotation marks for strings should be double quotes. DHH didn’t add this rule but someone else did. That person also rewrote all existing code, which leads meaningless git blame output. I don’t think that rewrite was worth the confusion.

Also, RequireParentheses = true , which means requiring parentheses when calling methods, it seems unnecessary because it is not Java.

In Rails, there is SpaceInsideHashLiteralBraces = true , which means you need to insert a space around the content of a hash literal. I think this is not that popular amongst programmers. I personally want to make a hash compact, so I follow this rule which is a bit too oddly spacious for me.

In short, I don’t mind if others have different coding style from mine unless it is my own project. On top of that, if the repository owner has a specific rule on coding style, I will follow that rule.

It is the same when I make a pull request. I read the original code carefully and understand the context of the code. I try to find the coding style and follow it, then commit my code. This is like reading between the lines and a fun part of reading code. It is also an important skill for OSS developers to make efforts to create commits that fit into the existing context.

-There are more rules than I expected. Can you tell us about the story behind installing RuboCop and coming up with rules?

The first time we incorporated RuboCop was for eliminating tedious fruitless communication between programmers such as notifying about TrailingWhitespace while reviewing pull requests.

When someone in the Rails team sends a chat message to the rest of the team about the rule they want, and if we all agree on that, we enable a new rule. I didn’t realize that we had this many rules till now. We only had a couple in the beginning.

It might be fun to take a look at the first commit.

It was a lot more simple than now. From around that time, we added rules such as necessity of parentheses, Style/Hash, Style/TrailingWhitespace, and two spaces/no tab.

It started from a few rules. I thought they would be enough. Before I knew it, there were many rules.

I think this way of adding new rules can be a good example. When you actually incorporate RuboCop in a project at work, you can start with a minimal set of rules that all programmers agree on, then, gradually increase the number of rules. This gradual introduction to RuboCop is a good practice if there are people are who hesitant to use RuboCop on your team.

Standard Coding Rules

-There are companies that have their coding style guide available online such as GitHub and Cookpad.

I think they are trying to be more productive by having a style guide whether programmers like it or not. When the company makes the style guide open to the public online, programmers who are considering to apply for a position there can see whether they can follow their style guide. If they can’t, they won’t have to apply for a company that they know they won’t fit in.

-Do you actually feel the style guide is helping with productivity?

To be honest, if the majority of programmers in an organization didn’t really care about the difference of coding style, it wouldn’t make much of a difference. But some people tend to fall into thinking they are doing their jobs just by pointing out the coding style issues on code reviews. So the style guide can prevent that kind of situation and let programmers focus on production.

I don’t care whether there is a space in curly brackets for hash or not because both syntax are accepted in Ruby. So the style guide can avoid communications between programmers with different coding style background.

I feel having a style guide for Ruby might make everyone in the community happy. Something that is based on Ruby’s general standards, unlike Rubocop.

Back in the day, we had NaCI coding style guide written by Shugo Maeda, a Ruby committer. It is a little dated now. The 80-character-per-line rule was a bit extreme. Minero Aoki, a Ruby comitter, also had an interesting style guide. I still remember his preference on empty lines. He said spaces(not the space as a character but the literal space) was important in coding. His style guide was fun to read. It had its own flavor.

-Speaking of style guide, I heard the next version of Elixir will come with a code formatting system.

It is recently implemented. I heard it was not the project owner, Jose’s idea but someone else’s. It is like Go, which has a standard code formatter in the language itself.

-I feel that they can just change their language grammar instead of providing a standard formatter.

That will cause a big incompatibility problem. You have the freedom to write in your style, but Elixir has a standard format for the case you don’t have a preference.

At first, when José heard about this idea, he thought it was not like Elixir. But the person who came up with the idea pushed hard, so José decided to try it out. As soon as he tried it, he loved it. He said “Why didn’t I do this for the first version?”

He also said, “Rubyists might not like to be forced to follow a particular style, but once we incorporated the code formatter, I feel much better now.” I assume Ruby will not incorporate a code formatting system. But this is an interesting case.

-Are you using RuboCop for your own project? I know you don’t personally agree with its default rules, but would you want to check the code in pull requests automatically?

I don’t use RuboCop for my personal project. My project isn’t at that level to feel it is troublesome to check each pull request by myself, so I don’t see the necessity of RuboCop.

There are a couple ways to handle pull requests. One way is to communicate through review comments and perfect the pull request then merge it. But I usually omit that process. I merge it to the master branch first, then, rewrite the code in my style.

When the project is the scale of Kaminari, I just merge everything, then I fix the code. Not for the style though. It’s more for the logic.

Oh, but when Sutou, a Ruby committer, did that to me, I was like, “why didn’t you tell me?” Maybe, I don’t mind doing it to others but I don’t like when someone does it to me. I know I am selfish.

This Pull Request was from the story I just mentioned. This is something like super ugly hack and rewrites the string of $LOADED_FEATURES , which is usually unacceptable. Knowing that, I made a pull request with a comment: “I’m sorry this is really ugly, but this is the only way this can work.” Sutou actually told me “It’s a surprising solution.” After he merged it, he rewrote the code in his style.

Automating Code Review and Sider