1. 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 :)

        1. 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.

            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.

              1. 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

                  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.

                  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

                      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.

                      1. 1

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

                    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)

                                1. 6

                                  I think this paid-only support setup is fine overall, but I do have a bone to pick with the way the author is interacting with people.

                                  One of the ways we can contribute back to open source is by making quality bug reports that save maintainers time. So, imagine you’ve investigated some problem and want to share that information with the maintainer, only to be told “yes, that’s a support request, please open a [paid] ticket.” The system is fine — you just don’t share your investigation and move on — but the way it is communicated is not cool, in my eyes. If it is such a common problem for the maintainer, then they should consider a canned response that is both nicer and also explains the situation, like:

                                  Thanks for taking the time to report the issue. Unfortunately, we’ve found that having an open bug tracking system leads to a lot of requests for free support disguised as bugs, which consumes a lot of maintainer time, so we’ve decided to restrict access to paid users only. If you are a paid user please post in ${place}, otherwise we are not taking public reports at this time.

                                  1. 1

                                    “ Categorising code as either public or private is an attempt to reduce the costs of change by communicating stability.”

                                    The purpose is to hide internal functionality from those seeking to use an abstraction. It isn’t to somehow indicate what is stable or not.

                                    “It’s to reduce future maintenance costs by discouraging coupling to unstable dependencies.”

                                    I think this is a possible consequence, not its purpose.

                                    1. 1

                                      The purpose is to hide internal functionality from those seeking to use an abstraction. It isn’t to somehow indicate what is stable or not.

                                      Why hide internal functionality? Why not make it all public?

                                      1. 1

                                        Well it depends on what one is modeling, but as an example consider the Abstract Data Type, like a stack or queue. They are defined in terms of their operations, not their internal representation. That representation together with supporting functionality is hidden by definition.

                                        Alternatively, consider that you are modeling a phenomena using an object in an object-oriented language. That one can make requests that the object do something without necessarily knowing how it does it, is how one can model a complex process unimportant to the requestor. An elevator (modeled in software) might know how to move between floors, but how it does so is unimportant to the requestor.

                                        This perhaps goes right back to the invention of the subroutine by Maurice Wilkes in the early 1950s. Being able to decompose problems into small reusable parts that hide their implementation, is necessary to create complex programs. The alternative of a very long procedure that merely has jump or goto statements is surely unattractive unless the problem is simple or trivial?

                                        1. 1

                                          Making things hidden (private) ensures that you don’t break someone else’s code with your internal changes. It’s a contract between you and your user – they agree to the fact that they won’t be able to use the private parts, while you agree to never break the public interface, while still having the ability to turn the internal details upside down if you so desire. As others have mentioned, it helps to reduce the mental load too: the user doesn’t need to read into every internal detail of your objects/modules, it’s enough for them to just be aware of the public bits.

                                          1. 1

                                            Yep, it’s to allow changes, and changes are a measure of stability. If it could be guaranteed not to change, then you wouldn’t really get any benefit from making it private. I was trying to show that to the original commenter with Socratic questioning, but they managed to dodge it expertly.

                                          2. 1

                                            Because objects have rights. And from that perspective, the unnecessary word is not private, but public; why should a language ever have a default behavior of breaking isolation and integrity? When we allow objects to keep to themselves and be left alone, then we are able to more fully notice when we take improper advantage of ambient authority.

                                        1. 3

                                          There are many language features that are both documentation and enforcement. Private is like that. Anyone can come by later and change it to something more visible. There’s some wiggle room there.

                                          A friend once made an IDE plugin he called Thatcher which went through a Java project and made everything private that could be private. To me, it seemed like a great idea as long as people felt free to increase the visibility as their design changed.

                                          1. 2

                                            That does sound like a good idea. It’s like dead code elimination, but for interfaces. Nice name, too.

                                          1. 14

                                            This is (related to) one of the most frustrating parts of working with Kotlin for me: no package visibility.

                                            Kotlin chose to abandon Java’s package visibility modifiers, arguing that it didn’t really “protect” your code because anybody else could trivially write code under the same package name and then see all of your package-private code.

                                            But that’s missing the point. We don’t usually write “private” because we’re afraid of people seeing or using the code. We write it so that they don’t have to see the code. Having an interface that is as small as possible reduces the cognitive load of someone consuming your library/package/class.

                                            I feel like the cognitive load aspect is something not discussed as much.

                                            Aside: In Kotlin, the suggestion is to just use “modules” instead of packages if you want that kind of protection, since it offers “true” protection from consumers accessing the private parts of your sub-code. I hate that because it’s more effort to move pieces around between modules, to change the public/private interface of a module, etc.

                                            1. 3

                                              Historically, languages have been poor at dealing with levels of abstraction above the class. When they do deal with them, they often aren’t first class constructs. It’s a shame. One could argue that micro-services (and many other things like OSGi) came from the absence of these abstractions in languages. My sense is that language designers don’t want to commit to a deployment model. Sadly, protection loses too.

                                              1. 3

                                                Yes. It is a shame. I feel like Rust modules are pretty nice, but then, I also don’t mind Java’s packages at all, either.

                                                What I find uncomfortable is this recent trend of the language acknowledging the concept of a file when it comes to privacy/visibility (e.g., Swift and Kotlin). That just feels weird and wrong to me…

                                              2. 2

                                                Kotlin chose to abandon Java’s package visibility modifiers, arguing that it didn’t really “protect” your code because anybody else could trivially write code under the same package name and then see all of your package-private code.

                                                Interesting. My tendency would be to go the other way and eliminate private and protected, but keep package. Anything in the same package as a class that depends on implementation details of that class is easy to refactor at the same time as a change to my implementation, so I don’t gain anything much from private and protected that I don’t have from package.

                                                If you add a new class in my package, then you are effectively forking my package. That’s fine, it’s up to you to decide that the maintenance burden of doing so is worth it for the change that you want to make. If I refactor my package and break something that your class depends on, that’s your problem because you have a downstream fork of my package, not just something using the package.

                                                1. 2

                                                  Exactly! Every namespace is a precious resource to be maintained as neatly as possible.

                                                  1. 2

                                                    We write it so that they don’t have to see the code.

                                                    I didn’t mention it in the article, but the “scissor test” is a concept that I like in regard to this. The idea is that if you were to print out a file of code on to paper, there should be a line that you could cut through with a pair of scissors that separates the interface from the implementation details. So if you want to use the class, you only need to read up until the scissor line, but if you want to understand how it works under the hood you can continue reading further. The scissor line is basically where private starts.

                                                    1. 1

                                                      Kotlin, as you mentioned, has internal visibility to hide things between modules. I find it nicer than package-private since you don’t need to have one package with many classes inside (subpackages’ classes can’t access the package-private members of a class in a parent package)

                                                      1. 2

                                                        I forgot that Java has no concept of subpackages, which is also disappointing. Rust modules seem to be the winner, then, from my experience.

                                                    1. 3

                                                      I’ve always liked how Python does it: by convention a leading _ indicates that something is intended for internal use, and is not part of the public interface. But it’s there, and you can import it and use it if you need to. This has been useful for me on several occasions.

                                                      1. 2

                                                        Agreed. There are probably situations where it’s advantageous to enforce it strictly, but in my experience it’s perfectly sufficient to just make it obvious (with something like an underscore), with the added benefit that you still have a choice instead of the compiler taking choices away from you.

                                                      1. 3

                                                        Congrats Bozhidar and the rest of the team! RuboCop really shows that success is not a destination, it’s a journey.

                                                        1. 2

                                                          True that! Thanks!