Best practices as code using RuboCop(careers.velory.com) |
Best practices as code using RuboCop(careers.velory.com) |
if foo? then
blah
end
TIL (something not that useful)
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.
def foo(x)
self.bar = x
end
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.)
if some_verbose_condition && some_other_verbose_condition
do_the_thing unless excluded_case || other_excluded_case
end
if (some_verbose_condition && some_other_verbose_condition) && !(excluded_case || other_excluded_case)
do_the_thing
end
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"
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.
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.
In particular, a lot from the lightning talk resonates with me.
- 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`.)
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.
These would be great to share and popularize. Too many Rails shops do this badly!
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.
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.
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.
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.
Sure about that? I'm saying if it is then extracting as a method might be clearer. All depends and these are other options.
(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.)
(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)
Sounds like maybe rubocop needs “-east-coast” and “-west-coast” presets. :)
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.