My Beef with RuboCop
rubypigeon.com
rubypigeon.com
It seems that a lot of people think that one of the main goals for code linting is consistency, when that shouldn't be a goal at all. Consistency is merely a tool, the goal should be readability/maintainability. Consistency is just one tool that is available to us in persuit of that goal.
I've hit similar issues to those mentioned in the article and it absolutely infuriates me when I have to jump through hoops (or worse, reduce the readability of my code) to push well thought out code that communicates intent, only to fall foul of a robot that doesn't understand nuance.
Don't get me wrong, I think Rubocop is a great tool, it's just over-used, and that's in part because everything is on by default, and this seems to be encouraged by the general community. It seems that once programmers have found a solution to something they love to apply that solution to all places that it will work, regardless of the efficacy.
My preference, and I suppose suggestion, is to let Rubocop handle things it knows about with a high degree of accuracy, such as tabs/spaces for lead indentation, security issues etc. and leave anything that requires nuance or subtlety to the programmer who wrote it, and the educational opportunity of the code review.
After all, "Consistency is the last refuge of the unimaginative" :)
I don't use Ruby but I faced similar issues with Rust's clippy and rustfmt. But I still think it's better than the alternative.
Every time I'm tempted to override one of these links/formats because I feel like my way is better I like to remind myself that it's going to set a precedent for the other devs on the project, and it's only a matter of time before one of us does something that the others don't like and it's only a matter of time before we return to pointless arguing about coding style and inconsistent code formatting.
>Consistency is merely a tool, the goal should be readability/maintainability.
That's the thing though: consistency is somewhat objective (and enforceable), readability and maintainability not so much.
TFA is a good example of this: they argue that explicitly returning "nil" is more readable and better communicates intent but I suspect (once again, not being a Ruby coder myself) that other experienced Rubyists could respond that everybody knows that a method finishing without an explicit value ends up returning nil, and that adding explicit code to do is is just clutter and distracts from the important things.
The worst things people do in collaborative work or management are always explained by "well, X is objective and easy to measure, so it's our target".
I also use Go, and haven't had issues with the autoformatter there, but I believe that's mostly because there is little to no room for ambiguity around intent. You tell the compiler you want to return a Bool, and if you don't it's a compile time error. There's no "I dunno, maybe nil was an intended return value".
> Every time I'm tempted to override one of these links/formats because I feel like my way is better I like to remind myself that it's going to set a precedent for the other devs on the project
I think this misses the point somewhat. This isn't about changing the defaults of Rubocop, it's about not having some of those defaults in the first place for the places in Ruby where it's possible to express intent in a way that it's not possible to check with a linter.
> That's the thing though: consistency is somewhat objective (and enforceable), readability and maintainability not so much.
I'm still not saying that people should not use linters, just because you can lint something doesn't mean you have to lint everything.
> TFA is a good example of this: they argue that explicitly returning "nil" is more readable and better communicates intent but I suspect (once again, not being a Ruby coder myself) that other experienced Rubyists could respond that everybody knows that a method finishing without an explicit value ends up returning nil, and that adding explicit code to do is is just clutter and distracts from the important things.
Oh, absolutely, most Ruby programmers do know this, but it's not really about the code specifically I think, it's about the contract. Take this code:
def foo
@some_instance_var = 123
end
> a = foo
=> 123
> a
=> 123
def foo
@some_instance_var = 123
@some_other_instance_var = 456
end
We could fix this with (amongst other things):
def foo
return @some_instance_var = 123
end
1. If we are changing this method we need to respect the return
2. If we are consuming this method we know the return is deliberate
This unfortunately violates Rubocop's redundant return detection and will be rejected (as will other forms).
This is...an odd view to me. With the exception of the "?" convention for usable-as-boolean query methods (which might not be actual booleans but you shouldn't rely on anything other than the boolean sense of the return value) and methods which are specifically documented otherwise, in Ruby every return value is and should be treated as intentional, not something that "happens to happen", and explicit return is used to avoid convoluted control structures when no better way to do so exists.
Aren't the Ruby semantics clear about what happens in both of these cases (return values of methods and assignments) and hence it was the coder's choice to leave that as the return value?
An explicit return says "I chose to do this". An implicit return in these examples says "There may or may not have been a choice made" and that's the problem. If you rely on the assumption that a choice was made, you add risk.
Ruby is a tool that I use the way I want, not the other way round
rubocop -a --auto-gen-config --auto-gen-only-exclude
Your just added exceptions for the leftovers that didn't pass the initial lint. But obv use this only after you cleaned up anything that should be. -a by itself autofixes anything it knows how to.
I wasn't saying that linters were a problem, I was saying that the over application of linters was a problem.
In general, overly broad application of a tool is a problem, wether in programming or not. Linting is a tool, it is possible to over apply it, and I argue that this is regularly done with Rubocop.
> So we welcome the consistency and focus on something we care more about.
What is the goal of consistency?
i don't know anything about linters or rubocop, but i do think you are wrong on this narrow point.
consistency is good in and of itself, it is not merely a symptom of your true goal of maintainability or readability as you mention in op.
id prefer someone/something that is consistently wrong to someone/something that is more often right, but randomly wrong. consistency implies methodology that can be changed and adapted, as opposes to simple mistakes that have to be assiduously monitored for.
you can train a person into a different habit and refactoring is much easier than bug hunting.
It can't be, because consistency is only a concept with no intrinsic value. The value is based on the relationship between the concept and the application.
You can say "consistency in code is good" or "consistency in code is bad", but then you've made an argument and you need back that up with reason.
> id prefer someone/something that is consistently wrong to someone/something that is more often right, but randomly wrong.
Just to be clear, we're not talking about bugs here, we're talking about style where there's, sometimes subjectively, "better" or "worse" more often than "right" or "wrong".
> consistency implies methodology that can be changed and adapted, as opposes to simple mistakes that have to be assiduously monitored for.
Consistency doesn't imply anything of the sort, but if you want to apply consistency to a problem with the goal of being able to change every instance of something you're not really solving a problem, unless your actual goal is consistency, which I argue isn't actually a goal at all.
Let's say I have the following three methods:
def foo
if broken?
:error
else
@x
end
end
def bar
if broken?
5
else
@y
end
end
def qux
if broken?
nil
else
@z
end
end
Whether you use ' or " for strings, or the %d shorthand or fully formed arrays in Ruby has nothing to do with the correctness of your code.
When these are trivial, it's not such a big deal - perhaps a bit of friction for developers, but not in a way that slows them down significantly. Rubocop does occasionally get into style rules that can change what your code is trying to communicate though, and those can often have negative effects.
Rubocop and other linting tools do aid in readability somewhat, but I agree that a lot of the time they can either focus on trivial concerns that don't really make a large impact on readability (things like which quote characters to use), and in some cases actively harm readability (the examples from this article).
Personally, my feeling on tools like Rubocop is that where they break down is often more important than the areas where they help (high level structure and communicating intent), and often the benefits of consistency in syntax are frankly pretty marginal, but I can get people having different opinions on this. There's certainly Rubocop rules I enable and get value from, but I do have quite a lot turned off and don't really notice much impact from it.
Implicit return, whether you like it or not, is frankly a weird feature, and is sure to rub some people the wrong way.
Where I think rubocop really shines is in picking up _problem_ code, like potential bugs, performance issues, or errors, in advance. We also use it heavily when deprecating older modules/classes, like replacing JSON calls with our own wrapper.
It does, however, sometimes suggest something totally bonkers: https://gitlab.com/gitlab-org/gitlab/-/issues/271570, but it's fairly trivial to skip it. After all, humans should be allowed to say that the linter is wrong and either ignore it or suggest an improvement, and that's a process issue rather than a rubocop issue :)
Not everyone is going to agree with every cop you enable, but the goal is standardisation rather than appeasing everybody. If something is particularly contentious then I would suggest removing/disabling that cop.
Every few years at GitLab we do seem to get a singlequotes vs doublequotes cop argument crop up, which is why we (thankfully) still don't have any requirement around those. Team doublequotes represent!
Edit: Oh and unrelated to the content of the article, but I really like the design and colour palette of the blog.
So you randomly have single and double quotes throughout the codebase?
I don't care which one a project uses, but it seems wise to pick one. Uniform syntax is very helpful for code readability.
I'm personally part of the second group simply because my linter forced it, but at the end of the day, it's such a low impact we don't really care.
So in that case, they're not used randomly according to the individual user's preference. There's a consistent rule. It's helpful for readability, too. And not sure about Rubocop in Ruby, but many linters in other languages can pick up on these different applications.
Rubocop would have you use singles for non interpolated and double for interpolated.
I get that there is a potential to catch some kind of error where you use double quotes on a string that will interpolated something which it wouldn't if it were in single quotes. In 12 years, I've never had that happen. I have, however, had it happen dozens of times where I changed something from interpolated to not and then I have to bounce over to change the quotes. This happens a lot and is annoying.
It's also an eyesore when you have an array of messages, each neatly aligned, but some are interpolated and some aren't, so the quotes change.
In Ruby, knowing that a string has interpolation is easy enough by seeing the #{} in the middle of it. It's hard to miss, really.
The single/double quote cop which is turned on by default is also counter to the existing style of every-string-is-double when it came along.
irb(main):001:0> puts '1\t2',"1\t2"
1\t2
1 2
{} are not required for instance variables.
Fair enough. I'm not much into Ruby but if I were, that's what I would encourage. But I'd be ok with whatever house style, as long as it's consistent.
I don't understand this argument. Are people really having trouble reading code because it has two different quote characters? This goes against my understanding of "readability"? What is it that is really meant by this?
Add in programming styles and a flexible language like Ruby can look like entirely different languages from one dev to the next (Ruby is a very traditional OOP capable language, or a weird but capable lisp, mix the two and readability suffers in new and exciting ways).
Personally I have a few reasons why singlequotes are inferior:
- String interpolation requires doublequotes
- Using an apostrophe requires you to escape it or use doublequotes
- They're harder to type on a Japanese-layout keyboard, which I use (it's shift+7, lol), which is otherwise really efficient for Ruby
But there are arguments against doublequotes too:
- I guess they're maybe harder to type on an ANSI keyboard?
- They parse ASCII escape codes, and singlequotes don't
But in my experience I need to use interpolation and apostrophes more than I care about not parsing \r so I use doublequotes everywhere in my own codebase.
So I would just rather not force either, as people seem to feel strongly both ways and the added frustration is probably worse for productivity than having it look a bit neater :)
Honestly I think the "community" chose wrongly in trying to standardise on singlequotes-unless-you-need-interpolation, when it should probably be doublequotes-unless-you-have-a-good-reason, but it's pretty hard to change that now.
...when obviously the correct answer is to consistently use %.
/s
class SanitizingFilter < HTMLFilter
def render_element(name, attrs, content)
return nil unless safe?(name, attrs)
super
end
end
Maybe I find it easier because I started programming with Basic and based a lot of logic on GOTO line and then moved to assembler language where again it used JMP so I represent code with nodes and edges in my mind.
Also, the original code - which the author prefers - has a positive ratio of language keywords over content (or logic) i.e extra 'end' and 'else' which does not bring too much information and makes me read it twice just to be sure I did not missed anything.
Also, the longer I write code the more I find myself avoiding anything but single-line if/unless statement modifiers. And I avoid if/else in almost all scenarios.
For the first situation, most `if` branches fall into two categories: guard statements, or genuine branching logic. For the former, guard statement style greatly improves clarity and you only need the statement modifier form. For the latter, I find in almost all cases you either should be writing two methods (one for the true case, one for the false case), or the inner logic of the `if` should simply be its own method.
return nil if thing?
return true unless other_thing?
logic()
special_situation if
another_thing?
logic2()
For `if`/`else`, I avoid them at all costs. If there's exactly two cases, odds are I probably want to separate methods (and can write a short method that chooses between the two if necessary). In almost all other cases, I'd infinitely rather prefer a `case` statement that can use tabular alignment for clarity.
case foo
when bar then baz
when ham then spam
else qux
end
Generally, yes.
> that can use tabular alignment for clarity.
I hate you. I mean, not you personally, but please don't. Even when I'm using a monospaced font, that vast gulf of whitespace between the else keyword and the substance of the else clause kills readability, and since I actually now pretty consistently use proportional fonts for programming... (I mean, sure, if its an environment where you now elastic tabstops are available, tabular alignment is fine; if you're faking it with spaces, though...)
The point of stating an explicit nil in the article must be to reduce the chance of future changes erroneously leaving something on the stack, without realizing the implicit nil.
The early return in your version makes it much more obvious why it is written the way it is.
class SanitizingFilter < HTMLFilter
def render_element(name, attrs, content)
safe?(name, attrs) ? super : nil
end
endI don't even think that this is RuboCop's fault but Ruby's. Because you can write said code in a lot of different ways (like shown in the post).
Honestly, the problem is that rubocop was not designed by a community of experts. It started as the opinions of a singular person, one motivated to the point of writing a linter where none existed. As a result, it's heavily opinionated on trivial shit. Yes, you can configure it, but the out-of-box experience is overbearing.
I generally prefer https://relaxed.ruby.style which is ... a lot more chill.
Now if you personally don't like this feature you should probably use a language which feature is that you can write a thing only in one way and no other.
Standard was also complaining a little but omitting the explicit else branch made it happy.
Anyways you can customize RuboCop to your likings, use standardrb that comes with a lot of carefully considered defaults and uses RuboCop under the hood or don't use a linter at all and move on with your life.
For the examples in the post, by all means discuss as a team and agree to exclude them.
This comment comes with the the smell of the idea that there is a right and wrong way to format code, when in fact with, Ruby at least, there's a lot of nuance that can be added in the way the article describes.
There are legitimate reasons for expressing the same functionality of code in different ways to express intent, something that Rubocop unfortunately isn't able to judge.
> I would rather work in a team that agrees to follow some set of conventions than than one in which everyone does things the way they prefer personally.
These aren't the only two options, there is a middle ground, in which you use cops that solve problems that are unambiguous, such as tabs/spaces for initial indentation, which brings me to the next point:
> For the examples in the post, by all means discuss as a team and agree to exclude them.
My main problem with Rubocop (which is a great tool) is that everything is enabled by default (and this seems to be encouraged) and the tool is overused. To me Rubocop smacks of the classic programmer problem of discovering you can do something, and so applying the solution to all the places the solution can be applied. "Because we can, not because we should".
I'm sorry you smelled that; I don't believe there is one way to do format code, but the opposite. I am saying that working in teams it's convenient to work to the team's style rather than your own particular likes and dislikes. Mainly avoid getting into code review Jenga about style preferences.
1. Inconsistency causing maintainability/readability problems.
2. The desire to avoid discussions about style within a team.
If you're talking about 1. here then what you're talking about is consistency as a tool that can be used to further the goal of maintainability/readability, but as with all tools it can be applied too broadly to problems it doesn't suit, which I would argue includes the subtlety and nuance around the intent of some code rather than the function, and so to force consistency there, and to nullify intent, harms the persuit of the end goal.
Point 2. is in my experience so rare that I'm tempted to assume it's just a post-rationalisation used to justify the pleasing sensation programmers get from making things the same. However, assuming it does exist, just in companies I've not experienced, this isn't really a goal that in my opinion is best served by a linter, once you've got to the point that you've automatically fixed all the issues that are obvious (such as lead spaces, syntax errors, security issues etc).
Everything you're left with after this point is a programmer either expressing intent, or making an unsound style choice, and this is best solved in code review. Expressing intent is valuable, especially in a language like Ruby where there is room for nuance.
If you've got two developers fighting over preferred style, bring in a senior developer to educate about this and perhaps even bring it up in the employee review. If you've got two senior developers who disagree about the style of a piece of code, then they should be expressing their opinions in a non-argumentative and suggestive style:
Senior a> "I would prefer to see this code written like this, but this is a personal preference"
Senior b> "I wrote it like this to show x intent" or "I have amended the code as you suggested"
What about a world where style concerns aren't part of a code review? Style debates provide the superficial appearance of a code review while offering no useful review of security, architecture and intent.
I like a super basic stripped down rubocop, and instead of a focus on "style", a more considered focus on readability.
Anyway remember you're writing code, not poetry. The inclination to act as if you're writing the latter without any kind of cost/benefit analysis is a major reason I dislike working with ruby devs. If only I were actually writing for powerpoint slides rather than for concrete business ends!
One thing that annoys me (and I'm not saying that you're doing it here) is that the people who say it's not worth arguing about are usually the ones doing the arguing. Like, if it makes no tangible difference either way, then just let people do whatever. It literally doesn't matter. But that's not acceptable to the "it's not worth discussing" people, and suddenly it becomes very important to discuss consistency. And not just any old consistency. We can't just flip a coin. No, it has to be consistent with their favourites, which are correct and therefore not worth discussing.
My personal standard is that if somebody suggests a rule and nobody really opposes it then great, enable it. If there is opposition, then it stays disabled until one side can persuade the other. If they truly care about consistency then they are welcome to change sides at any time. Attempts to browbeat the opposition should result in some private feedback from their line manager. There are exceptions for things that matter, but a lot of this stuff just doesn't.
Last time I raised a formatting change with our team, I got replies like "the default settings have been agreed with the community"(a blatant lie) or "I wasted enough time in my life discussing formatting" (okay, this guy has to write C# mostly, the standard formatting is already a lost cause there anyway).
Or maybe, if the author has a custom style, they should make their own linter rules.
Or maybe, just maybe, be hardcore and don't use linters at all. If you treat your code style as a form of art unto itself (which is not a bad thing at all IMO, depending on what you create), linters can be detrimental — they sure strip your code of a lot of its character.
I don't think it's Rubocop's fault in this case - there's really no way that we can expect a linter to glean "what is this method really about". But structural issues like this probably don't make sense to use a linter for IMO.
Unless they couldn't, and the linter does a good job. ;-)
def enhance_payload(payload)
payload[:h] = thing_h
if situation_one?
payload[:a] = thing_a
payload[:b] = thing_b
end
if situation_two?
payload[:x] = thing_x
payload[:y] = thing_y
end
end
def enhance_payload(payload)
payload[:h] = thing_h
if situation_one?
payload[:a] = thing_a
payload[:b] = thing_b
end
return unless situation_two?
payload[:x] = thing_x
payload[:y] = thing_y
end
If you can't think of examples, then you haven't been doing much thinking.
This is a matter of idiom and convention, and I disagree with the author on the specific point in Ruby. Were it Python, I would agree with it. (Well, not on "guard clauses" per se, but the significance of an implicit vs. explicit nil/None return.)
It is certainly an issue for polyglot programmers that as well as differences in syntax and behaviorally-visible semantics, language communities have different idioms that impact the communicative semantics of code (having been deeper into Ruby before getting more into Python, it took my a while to stop writing Python with Ruby conventions.)
This kind of insight into the dev mind/process is interesting b/c it shows how code is actually consumed. It also reveals where the implicit logic exists, so we can consider if it should be more explicit - in this case if functions should declare return types.
Rubocop is awesome. The -A flag does the right thing and the lint fixes really do make code easier to read. Even the examples in this post make me even more convinced of Rubocop's awesomeness.
super if safe?(name, attrs)
In our case, we were permitted to disable Rubocop rules at-will via inline comments, which made it slightly bearable, but still onerous considering how slow our CI was (45+ minutes in many cases for a test run)
My preferred method is:
1. As a matter of productivity, all coders are expected to have Rubocop enabled in their editor of choice, for the quickest possible feedback loop.
2. Use HoundCI or equivalent to highlight Rubocop complaints in Github's pull request conversation. Then they may become a matter of discussion. Obviously, this only works if the team is mature enough to avoid bikeshedding.
3. Rubocop failures do not, however, cause a hard failure during CI.
4. Haven't set this up yet, but ideally I'd configure things so that perhaps some egregious Rubocop failures (ie, tabs vs. spaces, SQL injection issues, etc) would cause CI failures. Whereas more subjective style issues would simply be flagged for developer attention/discussion.
5. Pull requests to modify the project's .rubocop.yml are always encouraged. Again, culture/maturity come into play here. I've been on teams that bikeshed this sort of thing to death. My current team does not.
I've been using RuboCop very heavily for the last few years. I've found it very useful as a solo developer, but it also makes it much easier to work with other developers since we never have to waste time talking about style issues.
Here's all the ways I've integrated RuboCop and made it an amazing experience:
I use VS Code with the ruby-rubocop extension, and I've enabled the "Format On Save" option. (This uses Prettier for most other file formats.) This would be unbearably slow with vanilla RuboCop, but I get a huge speed boost with rubocop-daemon. (Especially with the bash wrapper script that I wrote [2].) So now every time I save a Ruby file, the file is auto-formatted instantly to correct any warnings. In case it can't automatically fix a warning, I'll still the warning right in my editor, so I can immediately fix it before moving on.
The second step is a git hook script in `.git/hooks/commit-msg`. I set this up to run RuboCop for Ruby, plus prettier and eslint for JavaScript. If RuboCop fails with a warning, the script retries with `rubocop -A` to automatically correct any errors. Then I run `git diff` to make sure everything looks good before committing the changes.
The final step is to run `rubocop` as part of my CI build, to make sure I didn't miss any warnings (or for any developers who haven't set this up on their machines.) This almost always passes, because the previous two steps usually catch everything first.
I disagree with a lot of the default cops, so I just disable lots of them. But I'm pretty happy with my current setup and RuboCop configuration.
I totally agree that linters would be awful when you have a very slow feedback loop, and you have to wait anywhere from 10 - 60 minutes before you get a notification for a failed build. But they can be really pleasant experience when the feedback loops are under 100ms and most of the warnings are automatically fixed for you. I now lean on it really heavily, and I've even started to take some shortcuts and save keystrokes, because I know how the auto-formatter will tidy up the code.
I was an intern at that time so didn't make a fuss about changing the config.
VS Code will still highlight those cyclomatic complexity errors in the editor, so at least you will be able to fix them (or turn them off) without needing to wait for a CI build to finish.
All the project I've seen had specific adjustments to blanket disable some rules (on the top of my head, ` > 0 -> positive?` for instance) and change de defaults of others (e.g. line length)
The goal is tailor it to your needs, unlike other linters who have a much more oppiniated and rigid approach on what is supposed to be "right"
If you run into a very large number of linting errors then you might naturally write code in an unidiomatic style, which unfortunately takes a while to adjust to.
But let's nitpick a bit. I find it interesting that the arguments made in the article are about the intention of the programmer.
Perhaps an explicit nil does signal to me that the nil is intentional. But does that mean that all implicit nils are unimportant? For a skilled programmer like the OP, it probably does mean that! But how often do I actually know who wrote the code I'm reading? Of course I can git blame, but maybe the author left the org before I joined. What kind of programmer were they? Are implicit nils important or unimportant to them? Perhaps it's easy to argue that they should be unimportant. But in reality, I don't know what the author of a particular method was thinking while they were writing it. In fact even an explicit nil might be unimportant. Maybe somebody replaced a `raise SomethingError` line with `nil` to hot-fix an urgent bug.
I guess from my perspective as a long-time Ruby developer, all nils are important. It doesn't matter what the intention of the programmer was. Ruby is very expressive and very dynamic - anything can return nil and there are a hundred ways to do it. If your nil is intentional, write a test case for it. If you have no test suite in a Ruby project, then may God have mercy on your soul.
The only reasoning I need to support my opinion -- Ruby debuggers are not good enough to support mid line breakpoints -- so if you want to break only when the condition is true -- which is a very common thing if you are maintaining or debugging code you didn't write yourself -- you literally can't without changing the code or using a conditional breakpoint and duplicating the conditional inside the breakpoint definition -- this is very annoying when tracing unfamiliar code and trying to figure out what invariants are relevant to a given scenario (or missing).
In my opinion - linters need an actual concept of a user that they are trying to help -- and ideally that concept should lead to objective opinions. Linters like rubocop that are just piles of functionality tend to be breeding grounds for time wasting arguments between insufficiently experienced developers over opinions that aren't supportable from any usefully pragmatic basis.
> Making the nil implicit says that the return value is not important. This method now reads like it’s implemented by avoiding side effects within super.
That's...not my understanding of idiomatic ruby conventions. The idea that a method is assumed to have no or inconsequential side effects and be called only for its return value with an explicit return, but to be called for side effects with an implicit one, is a Python idiom, not a Ruby idiom. As I understand Ruby idiom, it is basically "a method defined with `def` is assumed to have both significant side effects and a significant return value unless documented otherwise on either point, except that a method with a name ending in `?` is assumed to return a value meaningful in a boolean context and have no or inconsequential side effects." Concise function bodies with implicit nil returns are normal, including for functions called for their return value.
If you need an exception to Rubocop, that's available to you.
alias 'karen'="rubocop"