Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

deno fmt accessibility #3695

Closed
brandonkal opened this issue Jan 17, 2020 · 19 comments
Closed

deno fmt accessibility #3695

brandonkal opened this issue Jan 17, 2020 · 19 comments
Labels
deno fmt Related to the "deno fmt" subcommand or dprint feat new feature (which has been agreed to/accepted)

Comments

@brandonkal
Copy link
Contributor

brandonkal commented Jan 17, 2020

I really like the idea of "one formatting style". People spend too much time on these matters. I also currently use two spaces for JavaScript files as it is the Prettier default.

But thinking more about the accessibility issues with defaulting to a small indent size, deno should reconsider.

Ref: stylelint/stylelint#4246
https://www.reddit.com/r/javascript/comments/c8drjo/nobody_talks_about_the_real_reason_to_use_tabs/

It is hard to read code when it is scrunched to one side. We have less of a need for a small indent size given the move from callbacks to async/await.

Given the above, I'd like to see deno fmt consider defaulting to use tabs before 1.0. When editing a file with tabs, the user can choose whether an indent should visually be displayed as 2, 4, or even 8 spaces. Tabs would also be more consistent with go fmt which the tool is clearly inspired from. I'm not interested in starting a holy war on this though.

I'd also like to see the ability to set the deno fmt CLI arguments as environment variables so users could use deno fmt if they strongly disagree with the defaults.

@kitsonk
Copy link
Contributor

kitsonk commented Jan 17, 2020

Since deno fmt is configurable, I say we stick with our defaults.

@brandonkal
Copy link
Contributor Author

But do consider that two spaces is unreadable for many people (this even varies between codebases). The article changed my mind on that. There is a reason TypeScript emits four spaces. Indentation is important. But using spaces forces the visual preference on everyone. For instance, you are fine seeing indents as two characters. My visually impaired friend needs 4 to be productive. By using the indentation character Tab, both of you can be happy. You can even ignore the key itself and your IDE will handle things for you.

Still, I can understand your fear to rock the boat...

For that, see my second suggestion. Allow us to set a

DENO_FMT_ARGS="--use-tabs --arrow-parens=always"

environment variable. That makes things close-to-seamless for codebases that care to be accessible.

@David-Else
Copy link

David-Else commented Jan 17, 2020

Deno formatting defaults should stick to what Prettier decides, that is the point of Prettier, so everyone can have the same formatting. With 80 characters print width any more than 2 spaces messes things up. It is all a fine balancing act. 80 chars and 2 spaces indent works great on a 1080p screen so you can get 2 columns.

The only exception to using the Prettier's current defaults is of course... it should be single quotes by default! I assume people touch type, the single quote are so much better. I understand that the Prettier team are considering switching to this in the future, so maybe it would be sense to beat them to it :)

Prettier 2.0 (old)
Summary:

Things that appear to have consensus:
#4102: Change the default value for singleQuote to true

prettier/prettier#3503 (comment)

It does not really matter though, as there is a command line option to use single quotes in deno, but I just thought I would nerd out and mention it here :)

I have never heard of the 2 spaces being an accessibility issue before, so I don't think this is a mainstream opinion. I do applaud people thinking about accessibility issues though, it is important.

@brandonkal
Copy link
Contributor Author

@David-Else 😆 I very much like single quotes as well! The thing is it doesn't really matter there. Touch type with single quotes and prettier will save it as double. I don't have a strong opinion on tabs vs spaces. But it is more accessible.

If you have to use a very large font, you may set tabs to use one column. If you have a 1080p screen and the code isn't terribly complex, you'll set it to two. If there is a lot of branching or you have a 4K screen, you'll set it to four or even 8. This can all be done without changing the file contents which is a distinct advantage. As far as I can tell, prettier treats tabs as 2 columns when printing.

I also believe it depends on what you are doing. For front-end (where more information is conveyed by JSX brackets etc) I've never had an issue with 2 column indents and 80 columns. Now as I write a Kubernetes client with all of that complexity, increasing the visible indent makes it easier to follow the complex TS types and branching.

Still, it is worth looking at before Deno 1.0.

@ry
Copy link
Member

ry commented Jan 17, 2020

2 space indent and 80 columns is what we're sticking with.

@ry ry closed this as completed Jan 17, 2020
@Alhadis
Copy link
Contributor

Alhadis commented Jan 20, 2020

JSYK, I once gave myself legitimate eye-strain trying to follow 2-space indented code for a pull-request. I actually gave up, and have been contributing less to open source projects because of the prevalence of this horrible tab-stop width.

Not saying "please change the defaults", I'm only lending credence to what @brandonkal's been saying about accessibility. It's a very real issue.

*drops mic*

2 space indent and 80 columns is what we're sticking with.

Not when I read it. 😁

@hdodov
Copy link

hdodov commented Apr 12, 2021

Sad to see this issue is closed, because I like what the Linux kernel docs say on this:

Tabs are 8 characters, and thus indentations are also 8 characters. There are heretic movements that try to make indentations 4 (or even 2!) characters deep, and that is akin to trying to define the value of PI to be 3.

Rationale: The whole idea behind indentation is to clearly define where a block of control starts and ends. Especially when you’ve been looking at your screen for 20 straight hours, you’ll find it a lot easier to see how the indentation works if you have large indentations.

Now, some people will claim that having 8-character indentations makes the code move too far to the right, and makes it hard to read on a 80-character terminal screen. The answer to that is that if you need more than 3 levels of indentation, you’re screwed anyway, and should fix your program.

In short, 8-char indents make things easier to read, and have the added benefit of warning you when you’re nesting your functions too deep. Heed that warning.

I used 4 spaces, then 2 (because of Standard JS), then switched to tabs when I joined a team. The reason is that with tabs, everyone is happy because code looks the way they want to and can read it comfortably, but most importantly, nobody has to agree or even care about anyone else's preferred indentation size. It works universally. And working universally sounds like a good feature for an opinionated code formatter to have.

I'm all about consistency and opinionated code style. But I think that code style encompasses syntax preferences like semicolons or no, single or double quotes, etc. Indentation is more about accessibility than code style, and tabs are the obvious winner.

@retog
Copy link

retog commented Jun 21, 2021

Very sad, no reason provided by people without disabilities. Indent with tabs, recommend to display them with the width of two spaces and count them as two characters when checking for the 80 characters line limit. What disadvantages would this rule have? It would make the code significantly more accessible to many.

@kitsonk
Copy link
Contributor

kitsonk commented Jun 21, 2021

I think we need to reopen this issue. We have talked amongst ourselves that this is important consideration and have discussed allowing deno fmt to potentially support a minimalistic set of options aligned to what prettier supports.

Having this issue closed makes it look like we don't care about these things and aren't talking about them.

@kitsonk kitsonk reopened this Jun 21, 2021
@retog
Copy link

retog commented Jun 23, 2021

Thanks, @kitsonk dor reopening. I'm also "all about consistency and opinionated code style", so I would prefer a non-optional choice for tabs, which would allow everyone to read existing code without having to reformat.

@Alhadis

This comment has been minimized.

@nayeemrmn

This comment has been minimized.

@Alhadis

This comment has been minimized.

@nayeemrmn

This comment has been minimized.

@Alhadis

This comment has been minimized.

@hdodov
Copy link

hdodov commented Jun 23, 2021

Maybe Deno could auto-detect and pick one or the other. In a space-indented file, it's not likely to have a tab character, therefore:

  • If there's a tab character present - format with tabs
  • Otherwise - format with 2 spaces

It doesn't sound good to search the entire source file and there could be edge cases (using tab characters in a comment), but I'm throwing it out there.

@stale
Copy link

stale bot commented Aug 22, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Aug 22, 2021
@kitsonk kitsonk added feat new feature (which has been agreed to/accepted) deno fmt Related to the "deno fmt" subcommand or dprint labels Aug 23, 2021
@stale stale bot removed the stale label Aug 23, 2021
@skuridin
Copy link

skuridin commented Sep 9, 2021

Since deno fmt is configurable, I say we stick with our defaults.

Is it? Cannot find any documentation.

v1.14.0 was just released with the fmt configuration. Looks like this issue can be closed.

@ry
Copy link
Member

ry commented Jan 10, 2022

Happy to support tabs as an option (#13266), but we're not doing it by default.

@ry ry closed this as completed Jan 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deno fmt Related to the "deno fmt" subcommand or dprint feat new feature (which has been agreed to/accepted)
Projects
None yet
Development

No branches or pull requests

10 participants