Updating Rubocop to the latest

[cmrd-senya]

So there was a major release of Rubocop and we're still on 0.*.

There are a couple of caveats with updating to 1.* that need to be taken care of, but in general I don't see any major blockers for the update.

But I'd like to know if there is something I don't know about which makes things more complicated than they seem to me.

[SuperTux88]

Yes, I still have that on my todo list. The main problem with rubocop updates is, that we have that many errors, that it's hard to see if there are new errors that we might want to disable.

I recently found out about https://docs.github.com/en/repositories/working-with-files/using-files/viewing-a-file#ignore-commits-in-the-blame-view and I talked with @denschub on IRC that we can use that to finally clean up some warnings (and then ignore those commits that only changed code styles), because we didn't want to clean them up in the past to not completely destroy our history with commits that don't change anything relevant to the logic as that would make it harder to find where something relevant changed using git blame.

It always annoyed me to see all those warnings about code style when reading the code (and sometimes the code style is so bad that it even makes the code harder to read, so the warnings have a reason). And I also never liked that new contributors create their first PR and then get yelled at by the review bot, that they need to fix a code-style issue that they didn't even touch and that was already there, but now as they touched the line, they are responsible to clean up our mess.

So I plan to do some cleanups of existing code-style warnings to bring down the numbers a lot (and ignoring the commits as mentioned above), and then when we have a lot clearer view about warnings, I'll upgrade rubocop to the latest version and then I'll see better if we need to disable some stuff in our config because we want something different from the default.

[denschub]

So, here's a proposal. Let's end the codestyle discussion before it even started.

In my fork, I just pushed a branch that adds Prettier for all *.{css,es6,hbs,js,md,rake,rb,scss,yml} files. The Prettier config I'm using is very minimal. In fact, the only thing I even touched is the default line width, which I bumped from 80 chars to 120 chars - which mirrors what we've been using so far.

Here is why I personally think this is a good idea: Code style discussions are highly subjective, they never end, and they're absolutely unproductive. Style lints are in theory good, but in practice they can scare contributors away (as mentioned already), and they might not catch a lot of things.

I have a lot of professional experience with very large and very old codebases. Primarly, I'm thinking about the Firefox codebase here, which is a very old mess. A lot of time was wasted in "oh I don't like how this looks, and I prefer that style", trying to build that into a linting rule, and still never being satisfied. Not too long ago, Mozilal enforced automatic code formatting on most of the sources, using clang-format for C/C++, cargo fmt for Rust, and prettier for JS. We ran the autoformatting suite on the entire codebase once, committed that, and are now done.

The result is now a relatively consistent codebase, and also a lower barrier for contributors: Instead of being yelled at by a lint tool, they can just run the autoformatter on their code and be done with it, no manual work. Even better, lots of editors (like VSCode) have a "format on save" option, which removes manual formatting interventions altogether. Also, since those tools are pretty much running with the default configs (similar to my prettier example), there are no more codestyle discussions. Tool does formatting, done. No need to discuss.

Now, does that result in perfect code all the time? No, absolutely not. There always will be cases where manually formatted code "might" be "a bit" better to read, and the autoformatting tool will ruin it. But in reality, that doesn't matter. It has no measurable impact on the actual ability to read and understand code, and to be quite honest, codestyle discussions are always a giant waste of time. Projects should spend their time on discussing what their code should do and how it should do it, not how the code should look like. :)

You can browse how an autoformatted diaspora codebase will look like in my branch: https://github.com/denschub/diaspora/tree/the-great-reformatting

Please take some time and maybe scroll over the large diff, or look at a couple of files. If there are cases where you think "hm I don't quite like that and I think this could be done better": Ignore that. As I said, in my opinion, that's a waste of time. If, however, you think there are cases where the autoformatting resulted in code that might be misleading on a first read, please highlight those cases.

Please also note that this isn't a PR to be merged. There are a couple of issues:

• Prettier highlights some syntax errors here and there - like unclosed tags in template files, duplicate keys in YML files, etc. Those things will have to be fixed manually. What I commited there is only the result of autoformatting - zero manual work.
• There probably are some file types missing. .erb, for example. Unfortuantely, the only erb-prettier-plugin that exists is outdated and doesn't quite work, and formatting it as a HTML file creates a bit of a mess. I think this is fine in the case of .erb, since we're eventually™ moving away from that anyway, I hope. If I missed other filetypes that you think we should force-format, let me know.
• The HAML autoformatter somehow forgets that the do-not-sanitize-prefix ! is relevant. This probably has an impact to the code.

If everyone actively working on the code agrees that we should use autoformatting, then I'm happy to drive this project to the end. If not, that's cool for me, too. It only took 30mins or so to set this up. ;D

---

Potentials style problems:

• Unit value list in .scss-lint.yml has their comments in the wrong place.

[denschub]

Update/edit: I removed .haml autoformatting from the branch. I filed a bug with the prettier plugin to let them know that it unfortunately removes force-unescapes, and that will break our stuff. Luckily, HAML will just throw syntax errors if it's off too much, and we have haml-lint anyway. So not running it through prettier is fine, imho.

[Flaburgan]

Hello, thanks for your time trying this.
I agree that coding style isn't even going to suit everyone, so probably we shouldn't discuss it too much, and I'm fine with autoformatting. I had a quick look at your diff and I think I'm fine with it, the only thing I'm not used too it the trailing comma after the } (like here for example). I don't know if I'm the only one not used to it?
.scss-lint is probably a good example of code which would be easier to read if not auto formatted (I'm talking about the units with the comments, here) but I guess that's the price to sometime pay when you have auto formatting and I'm fine with it.

However, what Pronto does is much more that formatting. For example, with our current settings, it asks to not used ids as CSS selectors because it may cause performance issues. In reality we are far from this level of optimization, and if we want to respect this rule there are quite some code which has to be manually rewritten, and some times that's not even possible (because we are styling some markup coming from external plugins for example). So there may be some pronto rules we want to discuss.

[denschub]

I had a quick look at your diff and I think I'm fine with it, the only thing I'm not used too it the trailing comma after the } (like here for example).

Can you say which file that is in, and maybe quote the diff directly? GitHub is surprisingly bad with diffs that huge, and clicking your link just results in me landing on the diff, but all the way in the top... so I don't know what you're talking about.. :(

.scss-lint is probably a good example of code which would be easier to read if not auto formatted (I'm talking about the units with the comments, here)

Yeah, I've noticed that. We can either move the comments to the beginning of each section, like so:

    global: [
        # Font-relative lengths
        "ch",
        "em",
        "ex",
        "rem",
        # Absolute lengths
        "cm",
        "in",
...

or we can always keep our current formatting and encase the block in prettier-ignore blocks. Let's keep a list of examples like this. I edited my comment above to include a checklist. :)

However, what Pronto does is much more that formatting.

Yes! This isn't about removing the other linters - it's about removing the code style aspects. RuboCop, for example - which is our Ruby linter - can be configured to work alongside prettier, so that codestyle is handled by prettier, but RuboCop still does "logic-level" tests. A similar setup is possible with eslint, where eslint stops checking for style, but keeps checking for logic. A similar thing is possible with scss-lint - it itself isn't doing much style checks at all anyway, so maybe it just works without conflicts, but even if it conflicts, we can make it not do that.

[Flaburgan]

Also, rules like Identifier 'item_id' is not in camel case. may be causing a lot of troubles to new comers to solve. In the case of #8035 it is quite a big refactor (43 occurrences in 14 files) so I feel it shouldn't be fixed in that PR. How should we deal with that?

[Flaburgan]

Can you say which file that is in, and maybe quote the diff directly?

You have it in basically ever JS files, aspect_membership.js is a short one so it's probably easy to spot. There is comma added after the }:

    });
  },
});

which is something I usually don't do. But maybe I should.

[SuperTux88]

which is something I usually don't do. But maybe I should.

As the autoformatter is adding it for you, you still don't have to do it manually.

But the advantage of doing this that you don't need to touch the line if you add something below that, as the comma is already there.

[denschub]

Also, rules like Identifier 'item_id' is not in camel case. may be causing a lot of troubles to new comers to solve.

For once, note that prettier isn't touching object keys. Code like

var foo = {
  bla_blah: "hi",
  blahBlahBlah: "hi",
  BLAHHHH: "hi!!",
};

will stay that way. Now, sure, eslint will complain about that, which is what you're seeing. Cases like this are cases we just have to fix manually. But as @SuperTux88 said,

So I plan to do some cleanups of existing code-style warnings to bring down the numbers a lot (and ignoring the commits as mentioned above), and then when we have a lot clearer view about warnings,

so... we should just fix those manually. :)

You have it in basically ever JS files, aspect_membership.js is a short one so it's probably easy to spot. There is comma added after the }:

Ah, I understand! The motivation for that is that if someone adds another entry to the object/array, the diff isn't touching the previous line, which makes them easier to backport/merge/apply-mailbox/...

This is configurable, but in my personal opinion, this is just fine. I was also kinda irritated by it in the beginning, but I stopped caring eventually, since prettier just adds it automatically when I hit "save" in my editor... :D

[denschub]

Oh, and "someone should fix those manually" does not necessarily mean that someone has to refactor all the code. Just like prettier, eslint has the option to turn it off completely, or just specific rules, for a block of code. If we determine that refactoring away a non-camel-case key away is too much work, we can disable the camel-case-rule for that block. There will be cases where this is done, and that's a fine deliberate decision to make in specific cases.

[cmrd-senya]

I'm fine with the solution suggested by @denschub.

If at some point we realize that we're unhappy with a certain auto-formatting rule configuration, won't there be any problem with changing the rule and rerunning auto-formatting over the codebase?

[tclaus]

I like also the idea to get autoformat on save. We get rid of different styles in the same files. Happens a lot when working on old files.

[SuperTux88]

TL;DR: I don't like the result of using prettier for ruby, while I like what rubocop does and I think rubocop solves all the problems we have for ruby code, and we don't need an additional tool.

---

OK, I played with it a bit and thought a lot about it. First, I'm only talking about backend ruby code, not frontend (JS/CSS/HTML), maybe we need a different discussion for the frontend, but as this issue was about rubocop, I focused on the ruby code.

I tried to integrate prettier in the diaspora_federation code, because if we would add prettier here, I would also want to add it to there, so the code-style is consistent between these projects (when I started it, I based on the rubocop rules that already existed here, also so the code-style is the same between the two repos). I used the diaspora_federation repo, because that's currently a project with a clean code-style, because at the moment rubocop is 100% happy with it (as I used rubocop from the beginning). So it's easier to spot what would be different when using prettier.

First I needed to remove some rubocop rules, as they did exactly the opposite to what prettier does (aligned hashes, spaces inside blocks/hashes), but that's fine, I don't care if it's the one or the other way, I only did it the way it is now because it was in the rubocop config in the diaspora repo. Then I found some code that would end bad when prettier formats it, but I found a different way to write that code so prettier can handle it better. Then I formatted everything with prettier ... most parts are equally well, some are maybe a little better, some a little worse, but I don't really care about that, both (before and after) are readable and understandable perfectly fine. But then the problems began ... a lot of places I liked better before, but they were still acceptable with prettier ... but a few places were extremely bad with prettier and I didn't find any ways to fix them and make them readable even with prettier. If you have nested blocks in nested blocks in nested blocks ... that's perfectly readable if it's on multiple lines indented for each block ... but prettier just puts everything on a single line ... and a block in a block in a block with an if in it, that's just not readable anymore.

@denschub told me that there are comments you can put around code you don't want to get formatted by prettier, but they don't work for ruby for some reason, prettier just ignores the ignore comments in ruby files (they somewhat work in markdown files, but even there prettier adds newlines inside the ignore-comments, but at least the newlines don't break the parsing anymore, while the spaces prettier inserted before broke everything).

So in some cases prettier produces extremely hard to read code, and as a developer I can't do anything against it. So, I don't like prettier for ruby code. And it doesn't solve any problems we have (but it creates new problems). I like rubocop a lot, I'm extremely happy with it and I never had any problems with it, and it can do everything we need in my opinion. The problem we have in the diaspora code is, that we have extremely old code that exists from before rubocop was enforced and we never cleaned that up. But as I wrote in my first comment, I'm planning to fix that (It just wasn't done yet because nobody knew about the .git-blame-ignore-revs file). But we don't need prettier for that, rubocop has everything we need:

• rubocop can be used to autoformat/autofix code
• rubocop has enough rules to generate clean code
• it still allows some flexibility for the developer for cases where one or an other formatting maybe is better readable (sometimes a few manually inserted newlines can help A LOT with readability), as long as the code still follow the rules.

In my opinion a good developer can still decide if a code is readable, and prettier can't do that, it sometimes produces code that's not readable anymore. Of course, rubocop also can't decide if the autoformatted code is readable in all cases, but we as developers can read the code after rubocop formatted it and see if it's readable and if not, maybe add some newlines or change the code otherwise ... or add a comment to ignore a rule (but I never needed to ignore a rule about code-formatting).

If at some point we realize that we're unhappy with a certain auto-formatting rule configuration, won't there be any problem with changing the rule and rerunning auto-formatting over the codebase?

@cmrd-senya actually, we can't ... all you can configure in prettier is which files are formatted and how long the lines should be (and the 120 we have now as we also had this with rubocop produces sometimes extremely unreadable code ... while setting it shorter produces unreadable code in other cases). prettier works in a way that it basically parses your code, and then generates new code with what it parsed. So it ALWAYS reformats the code. With rubocop we can do exactly what you say. If we want to change or disable a rule, we can do this, and reformat the whole code (using the .git-blame-ignore-revs). For most rules where you have two options if you want to enforce one or the other format, I don't mind which way we enforce, to me it's mostly important that it's the same everywhere. So if you have a reason why you want to change something, I'm happy to change it.

With prettier we can't do that, prettier doesn't care what you want as a developer, it just does it's thing. It always tries to do as long lines as possible, which is problematic if you have very small but nested blocks (and you as a developer can't decide to make multiple shorter lines), and if you set the width lower for all files, it enforces all lines to be shorter, which then splits some longer lines into multiple lines, only because you have longer variable or method names. I think a good developer can decide if it's fine to have a longer line with longer variable/method names, or wants to split a complex line that does multiple things into multiple lines. I think this worked fine in the past and I don't think we have a problem here that we need to solve. I think as long as rubocop was happy with the codestyle I also was happy with it (but I'm not happy with what prettier produces).

I like also the idea to get autoformat on save. We get rid of different styles in the same files. Happens a lot when working on old files.

I agree, see my first comment here ... but I disagree that we need prettier for that. I'm very happy with rubocop and it also can be used to autoformat code. I agree that old code in this repo is in a very bad state and it often happens when you touch old code, that's why I planned to fix it, but we don't need an additional tool for that.

---

Again, I was only talking about using prettier for ruby, maybe prettier is still an option for the frontend (we use prettier for the frontend in the company, and while I personally don't have to do a lot with it, I think our frontend developers are happy with it). And I don't know the other codestyle tools we use for frontend so far, what they can do and how well they works, as I just don't have to do enough with the frontend.

But for ruby I think rubocop is all we need, it works well, solves all the problems we have, and in my opinion works better for the stuff where it overlaps with prettier, as it gives more flexibility and allows the developer to think themselves, and if you want to structure something differently (because it makes sense in some scenarios), it allows it, as long as you are within the rules. And if you don't want, you can still also just only use the autoformatter. I hate it if I, as a developer, think something about how I structure my code into multiple lines, so it's easier to read, and prettier just puts everything into a long unreadable nested line, and I can't do anything about it (unless ignoring the whole file).

So my suggestion is to move the discussion about prettier for the frontend to somewhere else and I'll just cleanup the ruby code using rubocop as I already wanted to do in my first comment.

---

If you want to have a look at prettier in diaspora_federation, I pushed the result of into my fork here. That code was perfectly fine according to our current rubocop rules before, and in my opinion prettier created multiple things that were easier to read the way it was before (especially have a look at the last commit what prettier still reformatted even I tried to ignore it).

[denschub]

As I said earlier, I will not be engaging in discussions about what is "right" or "wrong", or what is "better" or "worse". :) I have removed Ruby and Ruby-adjacent files from my branch, which now remains at formatting **/{*.css,*.es6,*.hbs,*.js,*.md,*.scss,*.yml} only. If someone thinks that's worth it, please open a new issue and I'll probably write CI-integration for that.