artsy / README

:wave: - The documentation for being an Artsy Engineer
Creative Commons Attribution 4.0 International
1.1k stars 120 forks source link

RFC: Accessible spacing #504

Closed pvinis closed 1 year ago

pvinis commented 1 year ago

Intro

This might sound as another tabs vs spaces pitch, and it kind of is, but this time it's not about an opinion that some people might share and others might not. It's about accessibility and readability.

The main difference between 2 spaces and 4 spaces (or 8 spaces for that matter), is that some humans prefer more spaces so that they can see the indentation better. I would not call my eyes tired yet, but getting there. I often have to use look again or hunt the indentation guide (like https://marketplace.visualstudio.com/items?itemName=SteveDowerMSFT.IndentGuides) or rainbow guide (like https://marketplace.visualstudio.com/items?itemName=oderwat.indent-rainbow) when looking at package.json or any typescript/javascript file.

One common argument against this is that "JS uses spaces for indentation, and 2 spaces is standard". I don't think that's a good argument, because it's not about what's "standard", it's about what's readable by us, the humans that work here.

Our editors have colors/syntax highlighting. Some people use them, some others don't. Some use bright colors, some use dark, some use pastel, etc. Our editors support many fonts. Some people use bigger, smaller, thinner, wider, cartoon-ier, whatever. Both of these are customizable. Let's making indentation customizable too. Our editors can display one tab as 2-wide, 4-wide, 8-wide, etc. Some people will use whatever is closest to the "standard" and others will use whatever is most readable to them.

Why are we hardcoding indentation? Imagine we hardcoded the font, or the font size! Imagine we hardcoded the syntax highlighting colors! It would be removed approximately 3 seconds after it was forced on us. Why is indentation different?

Proposal

We change our prettier rules to use tabs for indentation, and we add the editorconfig line in our ts repos.

Reasoning

Like I said above, we should treat indentation the same way as font, font size, syntax highlighting, etc. It's a personal preference, and it's not about what's "standard", it's about what's readable/customizable by us.

How this RFC is resolved

We'll collect feedback on this RFC from the team for a week. We'll consider this RFC approved if 50% of the engineering team actively approves of this change. Add a 👍 or 👎 reaction to this proposal to vote.

damassi commented 1 year ago

Strong 👎 We don't do that anywhere else, in any of our codebases. And its a lot to ask every dev at the company / OSS world to go into their github settings and adjust a buried setting so that the code is legible, otherwise, by default, everything gets blown out to 8 full spaces and becomes an unreadable mess.

Every Artsy codebase is two spaces. Ruby is two spaces. Even our python code in hokusai is two spaces. This is a suggestion that def reignites the tabs vs spaces argument (which would be better to not have on your way out).

Also, most people don't think about this stuff. They want their code autoformatted with prettier (or whatever!), things linted as expected, etc. They're not concerned with micromanaging their IDE. In this case however, there's a lot of implications for suddenly switching code from spaces to tabs, because it requires further steps and effort teamwide from developers in order to maintain the baseline status quo of readability on GH. Not to mention the brutal git changelog that would arise where 100% of the files are touched * n number of JS codebases.

pvinis commented 1 year ago

First, I wanna say that I fully expected this RFC to be denied, but I've been meaning to show my side of this argument for a while, and I thought that if there ever was a team that would, not necessarily agree, but consider this point of view, it would be the artsy team.

Of course, I do not want to start a flame war or any big disagreement among people, so I think I'll close this. But I would like people to just consider this, maybe for the future.

Every Artsy codebase is two spaces. Ruby is two spaces.

This was exactly what I was trying to say. This is means nothing besides making it sometimes hard for me to read.

Also, most people don't think about this stuff.

But some do, when it's making it hard to read.

Not to mention the brutal git changelog

That has never been an issue. Not when we moved from master to main, not when we removed words like blacklist, not when we moved to strict mode, etc.

Alright, closing. Not a useful RFC at this point and my intentions were not to start any kind of fire. I only wanted to express my opinion about this decision, and like any other RFC, we can have a nice discussion. I still feel this can be done, but I also see how this topic is a bit sensitive and would spark a lot of i love this or i hate this rather than fruitful discussion.