[HN Gopher] Best practices as code using RuboCop
___________________________________________________________________
Best practices as code using RuboCop
Author : ubuwaits
Score : 53 points
Date : 2022-01-21 13:14 UTC (9 hours ago)
(HTM) web link (careers.velory.com)
(TXT) w3m dump (careers.velory.com)
| willcipriano wrote:
| 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.
| ch4s3 wrote:
| 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.
| ljm wrote:
| 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.
| bitwize wrote:
| "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.
| willcipriano wrote:
| > 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.
| jjgreen wrote:
| 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.
| sigzero wrote:
| 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?
| jjgreen wrote:
| 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.
| epidemian wrote:
| > 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.
| wrs wrote:
| 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.)
| jjgreen wrote:
| 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)
| gurkendoktor wrote:
| 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.
| jjgreen wrote:
| Oooh, I'd not heard of that before, it looks more pep8-ey,
| rather less bossy -- thanks for the hint.
| faitswulff wrote:
| Looks like rufo is looking for maintainers:
| https://github.com/ruby-formatter/rufo/issues/272
| matthewmacleod wrote:
| 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!
| jjgreen wrote:
| 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.
| rapind wrote:
| 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.
| ninkendo wrote:
| 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.)
| Karunamon wrote:
| 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.
| [deleted]
| joevandyk wrote:
| Using "then" in ruby definitely isn't idiomatic and goes
| against convention.
| jstan65536 wrote:
| 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"
| quesera wrote:
| > 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!
| jaredsohn wrote:
| There is already https://github.com/ankane/strong_migrations
| brightstep wrote:
| 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.
| jaredsohn wrote:
| or a model method or private method in the class containing
| this code
| Spivak wrote:
| But why?!?! There's no way this conditional is a well-
| defined named concept and just adds indirection for the
| sake of line length.
| brightstep wrote:
| maybe two well-named concepts then? If you're unable to
| reduce it at all, I'd consider that a code smell.
| jaredsohn wrote:
| >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.
| d3nj4l wrote:
| 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.
| LanceH wrote:
| do_some_thing if
| ready_to_go(variables_needed_for_conditionals...)
| Spivak wrote:
| 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.
| shepherdjerred wrote:
| 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.
| dapirian wrote:
| 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 ;)
| ncphillips wrote:
| If the author is here, your text has no padding on mobile. Using
| an iPhone 12
| henryaj wrote:
| What's wrong with using `let`?
| [deleted]
| d3nj4l wrote:
| 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.
| r-s wrote:
| Some discussion here: https://github.com/rubocop/rubocop-
| rspec/issues/94
| ngcazz wrote:
| 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`.)
| weatherlight wrote:
| 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.
| hit8run wrote:
| I use standard.rb as a more opinionated and saner approach.
| engineeringmtm wrote:
| 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.
| aniforprez wrote:
| 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
| jstan65536 wrote:
| You should have a look at Standard Ruby
| https://github.com/testdouble/standard
|
| In particular, a lot from the lightning talk resonates with me.
| aniforprez wrote:
| It seems to use rubocop under the hood but enforces no
| configuration. Yeah that looks like something I can use
___________________________________________________________________
(page generated 2022-01-21 23:01 UTC)