Best practices as code using RuboCop | Hacker News Reader

Best practices as code using RuboCop

careers.velory.com

58 pointsubuwaits48 comments

I dislike rubocop, not because I dislike linters (pep8 is fine), but because the defaults have strong opinions about things that don't matter

    if foo? then
      blah
    end

will result in a complaint about how one should remove the "then". Sure, you can configure rubocop to not make that complaint, and then the next one, and then next one ... but whatever happened to convention-over-configuration? I choose the convention of not using rubocop.

Using "then" in ruby definitely isn't idiomatic and goes against convention.

Yeah I’m sympathetic to this argument and I myself am often annoyed by rubocop’s apparent heuristic of “is it possible to encode an opinion on this? Great then let’s do it” … but in this case, I think this rule is beyond justifiable.

Have used Ruby professionally for a few years now and had no idea "then" exists.

TIL (something not that useful)

Going through the Rubocop configuration ordeal is probably a good investment of time if you have a large codebase where every developer has a different style, and you don't want everyone's code to look completely different.

But most of my Ruby projects are tiny tools with a bus factor of 1. I find "rufo" as a minimal formatter quite nice for those (there are VS Code plugins). For the most part it normalizes '' to "" in code that I might have copied from somewhere else, but doesn't go on to lecture me that "if not" must be written as "unless", and all the other things that the Rails community cares about.

Looks like rufo is looking for maintainers: https://github.com/ruby-formatter/rufo/issues/272

Oooh, I'd not heard of that before, it looks more pep8-ey, rather less bossy -- thanks for the hint.

I tried rubocop once, 8 years ago or so, and it saw code like this:

    def foo(x)
      self.bar = x
    end

and complained that `self.` should be removed. Somebody ran rubocop with autofix. It changed the code to just `bar = x`, which is not the same thing (it just creates a new variable called bar), and it resulted in some really horrible bugs that made their way to production.

I never used rubocop again.

(I'm really hoping this was just a rubocop bug, and has since been fixed, but it's enough to ruin your trust.)

I don't understand this complaint – the convention is encoded in the defaults. If you actively choose configuration over convention, it doesn't make much sense to complain that you had to supply configuration!

I mean: the defaults are so dreadful and wrong that I would have to configure in order to use it; and I don't want to do that.

Saying style defaults are “wrong” is subjective (unless of course it actually breaks your code).

I don’t like rubocops defaults either though tbh.

Ultimately to be highly successful I think this has to happen at the language level, and early on (I.e golang).

Rubocop should still be useful for teams, but will probably annoy some of your members.

I have similar feelings about pep8's defaults on things that don't matter. It complains about comment formatting. The default 79 character line limit is also pretty unreasonable in 2022, especially when combined with the demand to use spaces instead of tabs.

It's right there in their documents about "then" being "bad". Did you just decided "I'll use Rubocop." and not look at what it's conventions were?

No, this first came up at a place I worked, someone suggested using it, we evaluated it, this was one of the things that annoyed me, because it doesn't matter. After quite a bit of time discussing and configuring it we decided not to proceed. From time-to-time I'll look at it again, but it seems to get more bossy as time goes by.

Enforcing consistent choices for things that don’t matter is most of the value of a code style linter. In fact “things that don’t matter” is not a bad definition of “code style”.

(BTW, “then” on a multiline “if” is definitely outside mainstream Ruby style, based on my decade of experience...this is not one of Rubocop’s controversial defaults.)

I'm not sure I agree, code style is (or should) be about things like method length, naming conventions, iteration styles ... those do matter and should be in the scope of a linter. The presence of the word "then" which makes no difference to the resulting bytecode but does make the code more readable (IMHO) is not.

(BTW, I've seen "then"s aplenty in my decade-and-a-bit of Ruby experience, possibly this is a geographical thing, like the SF no-parentheses-in-methods thing)

I guess one could distinguish between “code formatter” and “code style linter” and declare that anything that doesn’t affect the resulting binary (“doesn’t matter” in your definition) has to be in the former. But I don’t think I’d want such a strict division of tools. I think formatting does matter.

Sounds like maybe rubocop needs “-east-coast” and “-west-coast” presets. :)

> because it doesn't matter

But, if it doesn't matter, why not let RuboCop make all `if`s consistent with `rubocop --auto-fix` and be done with it, instead of configuring RuboCop to not complain about it?

It seemed that this style choice did, in fact, matter a bit. At least enough to make you change RuboCop's defaults to accommodate to it. And that's fine; that's why those things are configurable :)

Of course, it's also fine to decide not to use RuboCop if you need to reconfigure a lot of it's defaults. Fighting with our own tools doesn't make any sense, but for some reason it's not an uncommon thing to do in this industry.

A lot about the Rubocop philosophy really grates on me. Many of its preferences are arbitrary and don't, in my opinion, contribute to code readability. Many others are good as a rule of thumb but cause more harm than good when they are blindly enforced by a robot. A recent example from my work went something like this:

  if some_verbose_condition && some_other_verbose_condition
    do_the_thing unless excluded_case || other_excluded_case
  end

Rubocop changed this to

  if (some_verbose_condition && some_other_verbose_condition) && !(excluded_case || other_excluded_case)
    do_the_thing
  end

which is just worse

and then it had the gall to complain that the line containing the `if` was too long.

That said, if you disable half its rules, Rubocop can be a useful tool. We've long had a list of database migration best practices, which we've built up over the years to ensure changes to our application's database schema don't cause downtime or other issues. Lately I've been writing cops to automate checks against these practices.

Useful feedback: "Heads up: changing the type of that column is going to lock the users table and bring the site down; see $BEST_PRACTICES_DOCUMENT"

Not useful feedback: "zomg ur cyclomatic complexity si 2 high!!1"

I don't know, your version in the example looks pretty bad to me? Sure, maybe the excessive line length of the combined if is a readability issue, but something about a postfix unless inside an if makes it very hard to follow what combination of flags would trigger it. In this scenario, I'd try to give meaningful names to the boolean expressions, and write a simpler conditional.

do_some_thing if ready_to_go(variables_needed_for_conditionals...)

If this pattern just the norm in Ruby because I loathe these single purpose functions that are basically comments that I have to jump to another function to see. It makes tracing what actually happens in some piece of code impossible without a notes file to make it all on-screen.

Considering OP was a bit vague with:

if some_verbose_condition && some_other_verbose_condition do_the_thing unless excluded_case || other_excluded_case end Rubocop changed this to if (some_verbose_condition && some_other_verbose_condition) && !(excluded_case || other_excluded_case) do_the_thing end

I don't really know what the proper solution is. I present one possibility here. "verbose condition" may or may not be complex, I don't know. But it's definitely a candidate for putting in it's own function and making tests out of it if there is complexity, or if it's just hard to read.

I'll agree there are too many of the following in Ruby:

def red?(color) return color == "red" end

As we don't know what his conditions are, we don't know if a function makes sense or not. There are times when you want to see the conditions up front, and times when they are a mere afterthought and relegating them to a function in the guard clause is best.

For whoever is downvoting just because they don't like the style of code in a discussion of abstract code, there is always:

do_the_thing if a && b unless c || d

Nothing like double guard clauses for double the lint fun.

I don't like either of your or rubocop's approach. If you have that many conditionals, bind a local variable that describes the condition being checked. Then you have an easy to read `if` with a single variable.

or a model method or private method in the class containing this code

But why?!?! There's no way this conditional is a well-defined named concept and just adds indirection for the sake of line length.

>There's no way this conditional is a well-defined named concept

Sure about that? I'm saying if it is then extracting as a method might be clearer. All depends and these are other options.

maybe two well-named concepts then? If you're unable to reduce it at all, I'd consider that a code smell.

> We've long had a list of database migration best practices > ... > Lately I've been writing cops to automate checks against these practices.

These would be great to share and popularize. Too many Rails shops do this badly!

There is already https://github.com/ankane/strong_migrations

I much prefer the tool Prettier. Any aesthetic styling of code should be unconfigurable so that teams using the tool don't waste time arguing about using tabs or spaces.

Prettier does a fantastic job of this for many languages. RuboCop is of course still useful for catching things that impact logic/functionality/performance (e.g. the issue presented in the article), but it's not a great choice for enforcing code formatting since it is far too configurable.

It can be brutal to use rubocop in an old project. Here is how we did it.

We only lint the files that changed after the date we integrated rubocop : `git -c log.showRoot=false log --no-merges --pretty=format: --name-only --since="2022-01-01"`

Then we heavily customized `.rubocop.yml` to avoid the rules that were not auto-correctable.

It was still brutal for a couple of weeks but now, maybe 2 years later, everything is fine.

Is there a tool for ruby that is actually opinionated and doesn't have a sea of configuration options? Rubocop just has WAY too many options and configuration going on. Tools for other languages like black/flake8 and govet are quite opinionated and these prevent bikeshedding. A lot of the rules as has been mentioned by others on this thread don't properly analyze the code resulting in bugs when you follow their recommendations. I'm not sure if Rubocop does an AST analysis or does it properly cause I've had a similar experience

You should have a look at Standard Ruby https://github.com/testdouble/standard

In particular, a lot from the lightning talk resonates with me.

It seems to use rubocop under the hood but enforces no configuration. Yeah that looks like something I can use

Rubocop is awesome and amazing tool when working with other engineers who may not have a background in ruby and rails, therefor are unfamiliar with best practices or conventions.

100% agree that documenting the practices of your repo is a losing battle: automate it or don't bother. I don't think you go far enough here. Every file in your repo should minimally have an autoformatter and some kind of linter/static analyzer/validator set up. Even shell scripts, ci pipeline configs, dockerfiles, terraform, etc. I recommend https://docs.trunk.io ;)

What's wrong with using `let`?

I personally am ambivalent about it, but the argument against let is generally about keeping as much of the context of your tests inside it. The error message of the cop in TFA alludes to this when it recommends using the four phase pattern (setup/exercise/verify/teardown). That way, you can almost look at a test in isolation and understand everything about it, which may not be true for a complex let.

Some discussion here: https://github.com/rubocop/rubocop-rspec/issues/94

Using `let` carries the risk of increasing the cognitive load required to understand why a test is failing or passing if it's not used carefully.

- they bring example execution order into play, especially with nested contexts and nested `let`s shadowing other above.

- they invite DRYing up test code, making it really easy to couple unrelated tests together and hard to understand tests in isolation.

- the corollary to the above is creating a brittle test suite. (in any case if DRY is a footgun in production code, it's doubly so in test code.)

- they require you to divert your attention from the examples to see what the states of your test objects are going to be.

These pitfalls can be avoided if, for example, you favor building up your test object graphs inside your examples.

(@r-s That's not the same thing though, they're talking about the eagerly evaluated version of `let`.)

If the author is here, your text has no padding on mobile. Using an iPhone 12

I use standard.rb as a more opinionated and saner approach.

Anyone else cringe at the use of "best practices" like this?

I can tell you why I do. I first encountered the term 15 years ago or so when studying the medical literature on HIV/AIDS. At the time (might still be this way) the most effective treatment was the now famous "drug cocktail", by applying multiple drugs that were individually only moderately effective we found that HIV/AIDS patients could live a somewhat normal life. In fact the treatment worked so well that after a decade of treatment some people live the rest of their lives without any detectable viral load at all, they are in effect cured of the disease and no longer needed treatment. This is the best practice, as it results in the best outcomes statistically speaking. The life expectancy of HIV/AIDS patients went from a few short months after infection to on par with the general population. This was provable, and not really a matter of serious debate as the evidence is overwhelming.

The formatting of a line of code one way or another feels completely different than that. It feels like somebody with a blog prefers it that way. It really should be called "best preferences" or something.

I agree that "best practices" probably isn't the best name for what community linters are and do, but "loose consensus among some OSS contributors is that these are agreeable convention" is a bit wordy. I don't think people generally take it literally.

Not for the same reason as you, but to me 'best practice' means that you can't do any better. In this context, it's saying that if you do it any other way that this specific way, it's objectively worse.

I prefer 'good practices' or 'guidelines' but as far as something like Rubocop is concerned, I don't really agree that its default setup meets that standard. Without some careful tweaking of the configuration you're likely to end up with a codebase full of premature abstractions that exist for literally no other reason except to satisfy Rubocop.

There is a subset of Rubocop rules that does a much better job, in terms of identifying potential sources of bugs (e.g. calling non-TZ aware date objects) and replacing deprecated methods with their alternatives where possible. The tool is worth it for that, so long as you disable all the nonsense about method lengths, class lengths, number of methods in a class, etc.

"Best practice" in software, near as I can tell, means "what you will not get fired for doing". It means adhering to rules that the community has adopted, irrespective of whether those rules are a good idea even in the general case, let alone a specific instance where it might be better to contravene conventional wisdom.

> calling non-TZ aware date objects

That's a great example. What if I'm working in a embedded system with limited memory and I need to shave off a few kilobytes? What if time zones don't matter for my implementation, say I make a timer app and the only thing that matters is the delta between two times?

There are things that I think rise close to the level of best practices. For example your password hash comparison function should probably run in constant time, but a linter is never going to pick up on something like that.

Reply on news.ycombinator.com