7 ms·
Best practices as code using RuboCop
- jjgreen 5y agoI 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.
- matthewmacleod 5y agoI 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 5y agoI 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 5y agoSaying 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.
- gurkendoktor 5y agoGoing 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 5y agoOooh, I'd not heard of that before, it looks more pep8-ey, rather less bossy -- thanks for the hint.
- faitswulff 5y agoLooks like rufo is looking for maintainers: https://github.com/ruby-formatter/rufo/issues/272 https://github.com/ruby-formatter/rufo/issues/272
- Karunamon 5y agoI 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.
- ninkendo 5y agoI 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.)
- joevandyk 5y agoUsing "then" in ruby definitely isn't idiomatic and goes against convention.
- burke 5y agoYeah I’m sympathetic to this argument and I myself am often annoyed by rubocop’s apparent heuristic of “is it possible to encode an opinion on this? Great then let’s do it” … but in this case, I think this rule is beyond justifiable.
- theonething 5y agoHave used Ruby professionally for a few years now and had no idea "then" exists. TIL (something not that useful)
- sigzero 5y agoIt'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 5y agoNo, 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.
- wrs 5y agoEnforcing 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 5y agoI'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)
- wrs 5y agoI guess one could distinguish between “code formatter” and “code style linter” and declare that anything that doesn’t affect the resulting binary (“doesn’t matter” in your definition) has to be in the former. But I don’t think I’d want such a strict division of tools. I think formatting does matter. Sounds like maybe rubocop needs “-east-coast” and “-west-coast” presets. :)
- deleted 5y ago[deleted]
- willcipriano 5y agoAnyone 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.
- ljm 5y agoNot 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.
- willcipriano 5y ago> 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.
- aniforprez 5y agoIs 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 5y agoYou should have a look at Standard Ruby https://github.com/testdouble/standard https://github.com/testdouble/standard In particular, a lot from the lightning talk resonates with me.
- aniforprez 5y agoIt seems to use rubocop under the hood but enforces no configuration. Yeah that looks like something I can use
- jstan65536 5y agoA 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 5y ago> 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 5y agoThere is already https://github.com/ankane/strong_migrations https://github.com/ankane/strong_migrations
- d3nj4l 5y agoI 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.
- weatherlight 5y agoRubocop 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.
- ncphillips 5y agoIf the author is here, your text has no padding on mobile. Using an iPhone 12
- henryaj 5y agoWhat's wrong with using `let`?
- deleted 5y ago[deleted]
- r-s 5y agoSome discussion here: https://github.com/rubocop/rubocop-rspec/issues/94 https://github.com/rubocop/rubocop-rspec/issues/94
- ngcazz 5y agoUsing `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`.)
- d3nj4l 5y agoI 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.
- engineeringmtm 5y agoIt 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.
- shepherdjerred 5y agoI 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.
- hit8run 5y agoI use standard.rb as a more opinionated and saner approach.
- dapirian 5y ago100% 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 https://docs.trunk.io ;)