Rubocop-obsession: RuboCop extension focused on higher-level concepts

https://github.com/jeromedalbert/rubocop-obsession

by thunderbong • 2 years ago

87 32 2 years ago

henning• 2 years ago

The world runs on code that violates some or all of these rules. Following them won't fix any of the problems they have. It doesn't matter.

hrdwdmrbl3• 2 years ago

It does help at some margin to make code more readable. Humans just aren't that great at reading and understanding code. It has been said: "Write for humans, not computers because computers will understand what you wrote no matter how badly written". So if you're writing code on a team, do your co-workers and your future-self a favour and write code that's easy to read. It won't run any better or with fewer bugs, but it will be easier to understand later, easier to be understood by others, and "wishful thinking here": easier to debug.

codesnik5• 2 years ago

problem is that it does too much out of the box. Finding some sloppy formatting - ok. Fixing it automatically? even better. But rubocop tries to sniff "code smells", security problems, and it's not that great with it, not everything you can get about the logic and intent just from AST. There are so many rules that developers spent significant time appeasing rubocop and this sometimes takes away effort to make code actually readable by humans. Have you ever seen a tidy mess?

Even worse, it gives simple tools to write plugins to sniff more logic related stuff, and I've seen projects where developers really went overboard with it.

xtracto• 2 years ago

One of the things i miss the most from developing in ruby is the code quality instrumentation: metric-fu, reek, rubocop and lots of other gems that allowed us (in a previous ruby shop) understand and monitor the quality of our code like ABC, Cyclomatic complexity, duplicated code, etc).

Now in Typescript, I haven't found a lot of tools to calculate,visualize and track these stats. Metric-fu was amazing.

Alifatisk2• 2 years ago

Don't forget about Brakeman!

MrJohz• 2 years ago

You can get a lot of mileage out of ESLint + typescript-ESLint + various other plugins. It's more of a "choose your own adventure" kind of experience, but the defaults are a good start, and if you're looking for cyclomatic complexity and all that jazz, there's various plugins that can calculate that. And the typescript ESLint plugin is great because it uses the types as part of the hints, which is great for finding places where you no longer need optional chaining, or where a condition is always true/false because you forgot what types things were.

That said, I tend to take a much more lean approach to linting these days. I try to only include lints that are actionable, likely to lead to real bugs, and have no false positives. Everything else feels like it's better handled in code review. If you don't know why your function is too complex and difficult to read, I don't want you to rewrite it just to satisfy a robot that's following arbitrary rules! I want to be able to have a discussion with you about proper techniques for splitting apart code.

But there's definitely people who go the other way entirely and build incredibly strict ESLint configurations, so looking for those might help you if that's what you want.

erulabs• 2 years ago

I wonder if anyone here has a good set of Typescript/ESlint addons to suggest? I also miss my duplicate code alert from Ruby!

emmanueloga_• 2 years ago

I'm pretty happy with Biome's defaults [1]. From time to time, I disable some rules that are too restrictive (like "noExplicitAny", which I turn into info instead of an error).

1: https://biomejs.dev/linter/

Alifatisk2• 2 years ago

I'm getting this right away!

codesnik5• 2 years ago

Nice. Now we can probably ask to move there half of the too nosy rubocop rules from the main rubocop repo. And then to never install rubocop-obsession!

bioneuralnet2• 2 years ago

Right? I seem to be the only high-level SWE at my company who hates RuboCop. Some of it's fine (e.g. no globals, use x.count { ... } instead of x.select { ... }.count). But so many of the rules are obsessed with trivial details like the "right" style of blocks to use (do vs {}) or "" vs '' for strings. Pointless busy work. I think the dumbest is requiring "def (x=true)" over "def (x = true)". Seriously, why could that possibly matter??

hrdwdmrbl3• 2 years ago

Why does it matter when there is auto-fix-on-save? I just write whatever, hit ctrl+s and the code is fixed. The result: The code is "fixed" based on whatever rule and it doesn't cost me a thing. I don't think about it and I don't lift a finger.

But I do personally appreciate the tidiness of code-consistency.

skissane• 2 years ago

I think the dumbest is requiring "def (x=true)" over "def (x = true)". Seriously, why could that possibly matter??

For Python, I use ruff to autoreformat my code into a consistent style. The CI pipeline fails the PR if the style is violated it. Fixing it is a one-liner. Actually, if you use VSCode’s ruff extension, a zero-liner, since it auto-formats on save.

For Java, I use Diffplug Spotless for a similar experience.

One advantage of forcing the code to be auto-formatted, is it makes diffs cleaner - you eliminate diffs where someone just changed the code style, since the code style never changes.

viraptor• 2 years ago

I see that as: it doesn't matter, therefore let's do one consistent thing and you'll never get a person saying "this should be x=true". RoboCop forces it one way and I never have to have a syntax/formatting discussion again.

You may think it really really doesn't matter, to the point that whatever form should be accepted. But then, would you accept my "def foo (x =true ,y= false )"? I've seen enough PRs with that kind of code to know that people will submit it one day.

wutwutwat2• 2 years ago

`””` vs `''` isn’t just a style rule. In Ruby there are performance implications with double quoted strings. Strings in double quotes are interpolated for variable substitution, whereas strings in single quotes are processed literally, no variable replacement overhead. Is it going to matter for most code? Probably not, but good to know nonetheless imo.

codesnik5• 2 years ago

strange, I think default rubocop setting is actually to add spaces around = in default values in functions. https://www.rubydoc.info/gems/rubocop/RuboCop/Cop/Layout/Spa...

this is an override in your project's .rubocop.yml

For me it's a bit opposite, I use and appreciate rubocop for simple formatting, not all of the rules but some of them, where autocorrect works. Recently I reformatted a big old project which was written by pretty sloppy developers who didn't care about whitespace, and it's just easier to work with now.

But I truly hate when rubocop tries to be smart about method calls, amount of lines in methods and amount methods or some other "code smell" metric.

yes, the tool allows to check for it, but it's not THAT smart. I have reasons. I'd happy to explain those reasons on code review to a human, but I'm not that happy to appease a dumb tool.

So, please do it as rails does, use bare minimals, something like https://github.com/rails/rubocop-rails-omakase

and ask other developers to stay away from .rubocop.yml

imoverclocked• 2 years ago

It’s been quite some time since I’ve used Rubocop but I did fight an uphill battle to get it into place. One of the things I had a difficult time explaining was that rules subtly interact with each other. When one rule makes a change, it might simplify something such that another rule can then also make a change. Once I enabled enough rules, there were some pretty impressive chains that were whittled down to almost nothing… but the same application of those rules elsewhere caused hemming and hawing by the naysayers.

I think the best thing that these tools give, above all else, is a way to settle petty opinions in a team. It’s why there is value in rules that either conflict with each other or are configurable so as to meet the expectations of a given team.

RangerScience• 2 years ago

Yep. I introduced Rubocop at a past gig… only to watch my colleague write worse code playing whackamole trying to adhere to the rules.

I kept telling him that the whackamole is a sign to rework things at a higher level - that the Rubocop rule catches symptoms, and that he needed to address the cause, but…

PS - to be clear, I love Rubocop and swear by it. In that circumstance, I needed to introduce it differently, is all.

Gigachad2• 2 years ago

is a way to settle petty opinions in a team.

Yeah this is huge. So often I get my PRs rejected because one dev has their own personal preference for formatting something. With Rubocop I can tell them to go submit a PR themselves to make that preference a rule and we can all discuss it and enforce it / autoformat it going forward. But for now, it won't be blocking my change going out.

block_dagger• 2 years ago

Hopefully rubocop-rails-omakase [1] will pull in stuff like this. I've gone from custom Rubocop rules in various projects to just using rubocop-rails-omakase, which finally feels like a consensus of sorts. I think Standard [2] is harmfully redundant, personally.

[1] https://github.com/rails/rubocop-rails-omakase [2] https://github.com/standardrb/standard

caseyohara• 2 years ago

I'm curious why you think Standard is "harmfully redundant"?

stevebmark• 2 years ago

Rubocop is one of the worst Ruby tools there is. Flog and reek are also unhelpful, but Rubocop is style nonsense that doesn’t belong in a linter. The ESLint community used to be big on useless opinion based rule overload, until everyone realized how bad of an idea it was.

leoh• 2 years ago

Can you be kore specific? I haven't used it for a few years but found it yo be a perfectly competent and helpful linter at minimum.

ukoki• 2 years ago

# This cop checks for `next` (and sometimes `break`) in loops.
...
# - For small loops, you can just use normal conditions instead of `next`.

This rule will just end up with devs writing this:

    array.each do |elem|
      if condition
        if another_condition
          if yet_another_condition
            do_something
          end 
        end 
      end
    end

Rather than the much easier to parse:

    array.each do |elem|
      next if !condition
      next if !another_condition
      next if !yet_another_condition
      
      do_something
    end

jdalbert• 2 years ago

That cop is disabled by default in `config/default.yml`.

Rubocop-obsession: RuboCop extension focused on higher-level concepts

Related Stories