(Notes from Editor: This interview was held in Japanese by Soutaro Matsumoto, CTO of Sider, and translated into English for Medium.)
We had an opportunity to talk with Akira Matsuda recently. Mr. Matsuda is known as a committer of Ruby and Ruby on Rails. He is the founder of Asakusa.rb, which is a local Ruby community that is most active in the world and the chief organizer of RubyKaigi, the biggest Ruby conference in the world. On top of that, he is the Gem creator who developed Kaminari and ActionArgs. We won’t list all of his roles here because it will be really long!
There are several articles of interviews with Mr. Matsuda. But this time, we focused on some topics he hasn’t talked about such as code review and review automation.
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.
- RubyCI https://rubyci.org
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.
-I had a hard time reading the compile.c file. I could finally read it when I displayed with tab size at 8.
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.
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
In the past, I worked on a project which I incorporated a complicated
.rubocop.yml. It took too much time deciding on the content of
.rubocop.yml. I felt it wasn’t helping with productivity. In the end, I chose not to do anything with it.
After all, I settled down with
DisabledByDefault. If it is not set like that, you cannot execute
bundle update when a new version of RuboCop is released. They regularly add new rules, and updating new version makes our CI fail.
bundle update of the product is not good. You install RuboCop to improve your productivity, but it actually slows down your production.
-I think Sider can solve that problem. Because you are running RuboCop in the CI, you encounter this problem. If you separate RuboCop from the CI and let Sider takes care of RuboCop, you will not have this problem because Sider reviews only the changes made in pull requests.
In that case, there is a chance of taking responsibility for the change someone else made. The last person to edit the line, whose name will show up with the
git blame command, should commit tolerable code even when they are not the the cause of the issue.
-Or, just ignore the warning. With Sider, you can review the warnings generated by RuboCop and decide whether you want to accept RuboCop’s suggestions or not. If you think RuboCop’s suggestion is not appropriate, you can ignore it for the moment and move on.
Technically speaking, does that mean Sider automatically inserts a comment that demands RuboCop to ignore the warning? Or does that mean Sider has a database?
-Sider has a database. Sider handles lines moved by commits, so it knows how the ignored issues are moved.
With this Sider’s feature, you can enable RuboCop’s rules you are not familiar with. RuboCop’s suggestions are not appropriate sometimes, but some rules can be very useful.
You are right. When RuboCop is set to
DisabledByDefault, there is a problem where I miss rules that are useful such as security-related rules.
Now I understand how well-thought and well-designed Sider is.
-I heard the creator of RuboCop will join this year’s RubyKaigi as a guest speaker. The conference program says he will talk about RuboCop.
Yes. Bozhidar Batsov (@bbatsov) will visit Japan for the first time and will be speaking on the first day of RubyKaigi. Seriously, his talk will be one of the most anticipated events at this year’s RubyKaigi other than the talks on Ruby3. Ruby committers often discuss coding styles and RuboCop, so being able to exchange opinion with the RuboCop creator in person will be an impressive experience.
When Bastov gave a keynote at EuRoKo, a Ruby Conference in Europe, he talked about the design of Ruby of the future. So this RubyKaigi must be an excellent opportunity for him to speak with Ruby committers in person.
- EuRuKo 2017 https://euruko2017.org
- Ruby4: To infinity and Beyond https://speakerdeck.com/bbatsov/ruby-4-to-infinity-and-beyond
In the proposal, Bozhidar gave me for his presentation, he said, “I want to cover many topics. I don’t know what to talk about for 40 minutes.” I gave him my feedback and asked him to focus on technical contents for the developers rather than talking about the design of the language. Because, you know that is what the RubyKaigi is about.
pocke and I (soutaro) from Sider will give a keynote at this year’s RubyKaigi. It takes place from tomorrow May 31st to June 2nd. For those who can’t make it to Sendai, please stay tuned for the exciting news from the site!
For more information about Sider, please go to our website.