1. 21
  1.  

  2. 13

    I don’t have an overall positive opinion of RuboCop or zealous linting in general, but, @tomdalling, I don’t see why you, or anyone who feels similarly, wouldn’t just customize the cops/rules for whatever project, and move on. Is this article a complaint against the chosen defaults? RuboCop’s authors wouldn’t be able to satisfy everyone; no matter what default they chose for any given style preference, there’ll be some set of people that don’t agree. I think it’s okay for them to have chosen whatever defaults they wanted, since, for the most part, they provide a lot of flexibility for adjusting or omitting cops/rules.

    1. 6

      I don’t see why [the author would write this article.] Is this article a complaint against the chosen defaults?

      No. The article is a complaint about a perceived culture of developers obeying RuboCop rules too fanatically. The article promotes the idea of disabling or reconfiguring RuboCop rules that make code worse in practice.

      I learned that from the Disclaimer/Conclusion section at the end of the article. I do think the article would have been clearer if it had started with that message.

      1. 2

        The disclaimer section being the main focus of the article would have made it much much better in my opinion rather than being relegated as a footnote. People need to realize that as developers we have tools to get our job done but they are just that, tools. If a tool doesn’t serve our purpose it is 100% within our right as professionals to not use that tool in favor of doing the right thing especially in cases where the tool actually makes the end product worse. The disclaimer is a MUCH more interesting topic than the complaint about a single RuboCop rule but I think its lost in the minutia of the rest of the article.

        1. 2

          I think the value of Rubocop isn’t the zealot-ness but instead the set of consistency it applies, which I think refutes @tomdalling’s point entirely.

          If you don’t like the rules, you can change them , that is a major feature of Rubocop.

          The specific example he used, I actually disagree with, and think the cognitive load, and clarity of what Rubocop recommended is far better than what he was initially wanting to write. So in the a parallel universe where @tomdalling and I are on the same team, I’m writing code one way, and he’s writing code another, and that is exactly why Rubocop is a good thing, these differences are not only brought to the forefront, they are discussed and decided on and whether you disagree or agree, you both end up agreeing that consistency is worth more than either way, and then Rubocop polices that decision.

          To me this is a tabs vs spaces kind of discussion, and my answer is: I mostly don’t care, but definitely not both!

          1. 4

            Is it better to be consistently less readable, or inconsistent but more readable? What if it only drops you from being 90% consistent to being 85% consistent? Why are we not 100% consistent? Shouldn’t we always use hash rockets instead of colons? Why do we use both normal and postifx conditionals, both if and unless, when it would be more consistent to only use it one way?

            Rather than refuting my point, I think you’ve actually given an example of it. I don’t think we should try to be consistent for the sake of being consistent — consistency for the consistency god, cops for the cop throne. We should be consistent to the extent that it provides benefits. Anything else is cargo culting. That was my point.

            1. 1

              Your whole point was around readability, and understanding, and how rubocop spoils it. But Consistency of style is a major factor to readability (more major than explicit/implicit nil, or guard expressions).

              If I’m misunderstanding your point, I apologise, but are you stating that inconsistency is sometimes a benefit? I don’t think I’ve ever experienced that, nor heard that position expressed.

              All of those questions you raise, are exactly the kind of questions you should raise, and decide as a team what to do. Use linting to raise awareness of these decisions, don’t follow it slavishly. Think: “Do I like implicit nil… no, so lets lint against that”, and then the team can move forward, and when a new member joins and the linting fails on their first commit, you can have the discussion again, and even change your mind. Raising team awareness is rarely a bad thing.

              1. 3

                That’s sort of but not exactly what I meant.

                Everyone knows that RuboCop can make code better. It wouldn’t exist otherwise. But not everyone knows that RuboCop can make code worse, too. Lots of people see it as purely positive, with no tradeoffs. If you believe that, then you should enable all the cops and never turn them off. I wanted to show that it doesn’t work like that.

                It’s the same with consistency. Some people view consistency as purely positive, with no tradeoffs. Everyone knows that it’s good to be consistent, but not everyone knows that it’s bad to be over-consistent. We already understand this intuitively, which is why I asked about hash rockets and all the different ways that Ruby provides to write conditionals, but we’re not always conscious of it.

                Think about it this way. Let’s imagine that Hammer Man has a hammer, and he uses it consistently. Need to drive in a nail? Hit it with the hammer. Need to break a rock? Hit it with the hammer. Need to cut some wood? Smash it apart with the hammer. Need to beat some eggs? Stir them with the hammer. Need to turn off the TV? Throw the hammer at it. That’s consistency. He might look at his neighbour who uses a saw, a whisk, and a TV remote, and think that they are being inconsistent. They have to learn all these different tools, and the tools cost money, and they take up space in your house, when a simple hammer could have been used instead. That’s all true, but the mistake is in thinking that consistency is always good, and inconsistency is always bad. Hammer Man is being over-consistent, which is bad, and the neighbour is being adequately inconsistent, which is good.

                You can draw parallels between Hammer Man and the way that developers use linters. Now that I think about it, and if I were to be cheeky, I might draw parallels between Hammer Man and people who really like Golang.

                1. 2

                  Hashrockets? Here’s my anecdote: rubocop says you should be consistent and choose one of foo: bar or :foo => :bar for your codebase (I think it defaults to the former). This is fine as general advice for application code, until I start getting PRs for my rake task dependencies that rewrite them from

                  task :default => [:html]
                  

                  to

                  task default: [:html]
                  

                  The entire point of this notation is that there’s an arrow from the task name to the dependencies. The fact that this evaluates as a single-key hash from a keyword to a list is … incidental: if that syntax instead resulted in the creation of a Proc or an anonymous class or a giant mecha arthropod, I suspect that Weirich would have done his level best to work with whatever that thing was, to get the list of dependencies out of it. But (the unthinking application of) Rubocop has resulted in completely missing the point

                  Yes, you can turn it off or change the defaults or ignore the warning (unless rubocop is gatekeeping your CI builds) but the point is that it creates a presumption in favour of the “wrong” notation that senior developers (or, I suspect, just old developers in general) now have to spend their time working against.

                  [/rant]

                  1. 1

                    Yep, that’s the spirit of what I was getting at. The developer writes it a certain way then RuboCop changes it for the worse because it can’t know the original intent, which has both technical costs (worse code) and social costs (time wasted, pissed-off devs).

                  2. 1

                    I think I agree with the point you are now making.

                    Though your analogy is wildly inaccurate.

                    If we are talking Rubocop: Everyone on the team is already using ruby, so they’re all Hammer Man already. So really you are deciding on techniques of using a hammer (by your analogy), not anything to do with other tools.

                    • Banging things in with the handle is bad, don’t do that.
                    • Use the back for pulling out nails.
                    • Use the front for hitting things.

                    This kind of consistency is always good, and there is no such thing as over consistency in this context.

                    1. 1

                      It wasn’t an analogy, it was a parable that shows there is such a thing as being too consistent.

                      If we’re talking about Ruby being analogous to a hammer, then there would be a RuboCop cop that bans the use of the claw because it’s not good at hitting things, and if you want to use the claw to pull out nails people will complain about how using two different sides of the hammer is not consistent, and it’s best practice to only use the front side.

                      There is such a thing as over consistency in this context — like only using one side of the hammer. I just don’t understand this desperate need devs have to treat consistency like some divine, flawless god that can never be wrong and can’t be questioned.

        2. 1

          It is explained at the bottom. I think this is a case of complaining about the article without having actually read the article. We all do it sometimes :)

        3. 6

          Rubocop prevents many issues at the cost of having to sometimes tell it “this is an exception to the rule”. Fortunately you can quite easily tell it that at various levels of granularity and the effort it takes is well worth it IMHO.

          1. 5

            I agree with the author but I don’t think the things he’s describing are specific to RuboCop. What RuboCop recommends in the first example is stylistically consistent with a lot of the Ruby ecosystem, and I would expect that many Rubyists I know would consider RuboCop’s recommendation to be more idiomatic than what the author (and I) prefer. This is my beef with Ruby in its entirety.

            1. 3

              The problem starts when it is viewed not as a tool, but as a set of divine commandments. Thou shalt not make implicit nils explicit. Thou shalt not write long-form conditionals. Thus saith the RuboCop.

              I completely agree with this. I’m not sure why, but I have internalized that disabling/ignoring a cop is a bad thing. It is not always a bad thing. Your article is a great example that it is okay to disable cops when they make your code harder to read and maintain.

              As engineering leaders, we should all try to tell our teammates that it is okay to ignore RuboCop when code is easier to understand ignoring a cop.

              1. 2

                You get it. I just want people to make RuboCop work for the team, instead of making the team work for RuboCop. If we’re going to put up hurdles for developers to spend time jumping over, then they need to provide a proportionate benefit.

              2. 3

                Folks who find frustration with RuboCop’s defaults might want to give standardrb a try. It wraps RuboCop with its own set of rules, which I personally prefer.

                1. 2

                  I’ve worked on projects which mandated use of various code formatters and linters, and without fail they’ve all had, in my opinion, a negative impact on code readability.

                  1. 19

                    My experience has been the exact opposite. In fact, I currently hold that collaborative development without mandated formatting will be filled with pointless arguments about silly details that an automatic formatter would just fix permanently. It’s like yet another thing no human (except for the few who decide the standard and author the tooling) should have to ever think about again.

                    I think Go perfected this approach from the beginning, and most others have followed suit, more or less. The longer a language has existed without de facto formatters, the worse the situation is. Of course, languages like python are a bit of a problem, since formatting a piece of python code is not exactly a deterministic problem. Black does a pretty good job though.

                    1. 5

                      I’m okay with all code formatters, as long as they don’t put record separators on the start of the following line. :D

                      1. 3

                        collaborative development without mandated formatting will be filled with pointless arguments about silly details that an automatic formatter would just fix permanently.

                        I used to think this too, but then I realised it depends on what the composition of your team is like. I’m currently in a team with very experienced engineers who basically always run issues through a mental cost/benefits model. We don’t have pointless arguments about silly details.

                        We tried using a linter and auto-formatter for a while, but we found that it only increased the time we spent on silly details. Sure, we got a great feeling of progress by fixing lint issues, but from a bigger perspective, it was just a game of chasing a lower lint count, and not providing any actual value.

                      2. 4

                        I feel link linters pull you towards a certain level of readability. Whether they pull you up or pull you down depends on where you started at.

                        After using a linter for a while, learning from the feedback it’s giving you, you end up writing code that already satisfies the linter before it gets linted. At that point, the linter only ever gives you false positives: when you’ve broken the rules deliberately because it makes the code better.

                        1. 2

                          I agree with your assessment. The problem arises when linters are run on the CI or a pre push hook and you can’t ignore them.

                          Currently fighting jsPrettier which is made to be as unconfigurable as possible. They call it opinionated. Not a fan.

                        2. 3

                          while I do think this is true of the RuboCop defaults and the AirBnB eslint config, I don’t think this is true of all code formatters/linters (I like PEP8 and gofmt, for example). Code formatters help to identify and keep consistent the values and styles of a community. The end result of this is that those communities, by nature of preserving their own values and styles, wind up excluding divergent values and styles, which … is the entire point. The entire point is to exclude differing styles. If you really don’t like a code formatter or linter on a project, that’s a hint that you either need to learn to adjust your practices to work in harmony with the people already on the project, -or- that the people working on the project don’t share your values and you might be better off working on different projects with people who have values more similar to your own.

                          1. 2

                            I’d love to use good formatters/linters, but most languages simply lack them.

                            So I rather use none than a bad one.

                            1. 1

                              I ageee that readiblilty is worse than for “ideal formatted stuff” but coworkers like it (being a team player = free brownie points for later) and it is nice to not have to futz about with layout cuz it will get mangled anyways.

                              Acceptance leads to piece of mind (for me anyways)

                            2. 2

                              I agree wholeheartedly with the author’s opinion on explicit vs. implicit nil.

                              On the point of guard clauses we disagree.

                              Except in this case, we’re not “guarding” the rest of the method from being run. That conditional is the entire raison d’être of the class.

                              And indeed, the raison d’être (execute super when appropriate) remains the same – albeit with a ruby-idiomatic postfix conditional.

                              Conditionals need not look a certain way. They don’t exist to lend a certain visual element to your code. Furthermore, Ruby is to blame for guard clauses, not Rubocop. It’s not unreasonable for Rubocop to recommend programmers express problems more succinctly. In my opinion, guard clauses make code more readable in code bases where they’re idiomatic. That’s an important caveat. If the developers you work with regularly ignore this Rubocop rule, then guard clauses will not be idiomatic in your code base, and you should avoid them for that reason.

                              1. 2

                                I wish the example wasn’t about ‘nil’ because the author seem to imbue ‘nil’ with some magic readability that I don’t think it has. It’s pushing the behavior back to the caller and having it decide what to do and I think that’s rarely a good idea.

                                1. 1

                                  Yeah, for me it isn’t about nil specifically. Sometimes things read more clearly with explicit returns.

                                  1. 1

                                    The behaviour is identical in all the code examples. They all return nil under the same circumstances. The only difference is whether you can see it actually written in the code.

                                  2. 2

                                    My current opinion on guard clauses is that they are appropriate in specific situations (a couple spring to mind). I think it’s a false dichotomy to either try and use them as much as possible, or not at all. I can see an argument for never using them, depending on the people in your team, but personally I’m not on those kinds of teams.

                                  3. 2

                                    Rubocop has bad defaults (methods are limited to 10 lines max, really?) and no syntax for disabling checks for a block of code (so no exceptions are allowed).

                                    1. 4

                                      You may disable cops inline via magic comments.

                                      # rubocop:disable Layout/LineLength, Style/StringLiterals
                                      [...]
                                      # rubocop:enable Layout/LineLength, Style/StringLiterals
                                      

                                      https://docs.rubocop.org/rubocop/configuration.html#disabling-cops-within-source-code

                                      1. 2

                                        Now, if LineLength will be disabled in config, it will remain enabled in the part of file (or even multiple files?) after that closing comment. It’s not proper “disable in block” instruction.

                                        1. 2

                                          If you use a postfix comment like:

                                          def some_method # rubocop:disable Layout/LineLength
                                            […]
                                          end
                                          

                                          or:

                                          class SomeClass # rubocop:disable Layout/LineLength
                                            […]
                                          end
                                          

                                          it is block-specific. (Layout/LineLength is an ironic one given how postfixing these can make the lines quite long, but I’ve never faced that problem.)

                                      2. 3

                                        It does have magic comments to disable checks. I find that sort of annotation distasteful, but it’s there.

                                        It has bad defaults - on that I agree. Which might be something to be looked-past, except how under-my-skin it is that those defaults are dubbed “the community’s guidelines” which I reject. It’s @bbatsov and the other maintainers’ curation based on participation in their issue trackers. That’s not the community. That’s A community, but every place I see rubocop, there’s exceptions that people configure. It’s not laid to rest, self-evident, universally agreed - any of that. What I consider a REASONABLE default would be to have tons of rules available, but only a few core ones enabled to start. There’s an “omakase” argument to make, sure, but at least don’t pretend to speak for the entire Ruby community.

                                        I liked this line from the article along these lines:

                                        The problem starts when it is viewed not as a tool, but as a set of divine commandments

                                        In fairness there WAS a survey they did about the defaults in May 2020 that got about 800 results. Several of the results that were shared showed how “unsettled” these formatting decisions are across the community. It was also shared here

                                        we’re definitely going to tackle the cop presets idea at some point and provide a smaller set of “essential” cops, alongside the current somewhat heavy-handed default set of cops.

                                        Edit: also want to be clear that I like code formatters - golang’s is great. I mostly think Rubocop goes too far. It’s a great thing to exist in the Ruby ecosystem, though, and I very much appreciate the hard engineering work put into it.

                                        1. 2

                                          There are things that I don’t like about the defaults, but I don’t know that @bbatsov has necessarily made the wrong choice. For example, there are practical reasons why I prefer trailing commas, and all the counterarguments I’ve seen are some variation of “it doesn’t look pleasing to me”, but there is no denying that it’s a minority opinion (roughly 1 in 4 according to the survey). So it is wrong to make no trailing commas the default? I don’t know that it is. Where is the cut off point for a majority opinion? 75%? 50%? 95%? It’s hard to say.

                                          Personally, I would prefer that the defaults only include things that have >90% agreement. As commenters have not been shy to point out, if people want the rules to be more strict they can edit the config and stop complaining about it. It cuts both ways.

                                      3. 2

                                        Actually I find the rubocop version more readable, if I have a beef, it’s with code that sometimes works and sometimes returns nil.

                                        Usually I refactor that to always work and the branch handled at a higher level.

                                        That said, it’s amazing how many rubocop style gems there are now.

                                        ie. Nobody can agree on style or readability.

                                        ie. If the experts cannot agree, who am I, mere me, to agree with them?

                                        1. 1

                                          Guard clauses are for bailing out early when you know that it’s not necessary to run the rest of the method…. Except in this case, we’re not “guarding” the rest of the method from being run. That conditional is the entire raison d’être of the class. It’s the most important part.

                                          “Line of sight” is a shockingly useful readability trick, especially when practiced across a code base.

                                          (Note: Article discusses Go, but the concept is not language-specific).

                                          TLDR:

                                          What gets get guarded shouldn’t be a subjective exercise in what to emphasize. Instead, you guard “anything not the happy path”. And the happy path is everything after the guards.

                                          Not only does this prevent nesting, but you can “scan” the function and see all the “weird cases” at a glance, and then the main case.

                                          From this perspective, the suggested guard makes sense:

                                          1. Not safe (weird case) = return nil.
                                          2. Safe (normal case) = apply filter.
                                          1. 0

                                            Can’t argue with that