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

tools: remove ESLint max-len rule #41509

Closed
wants to merge 6 commits into from
Closed

Conversation

Trott
Copy link
Member

@Trott Trott commented Jan 14, 2022

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. tools Issues and PRs related to the tools directory. labels Jan 14, 2022
@tniessen
Copy link
Member

Does this also remove all width restrictions on comments etc?

@Trott
Copy link
Member Author

Trott commented Jan 14, 2022

Does this also remove all width restrictions on comments etc?

I believe so. It does not remove the restriction on markdown file prose text because that is linted with remark-lint rather than ESLint.

@Trott Trott added the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label Jan 14, 2022
@tniessen
Copy link
Member

I 100% agree that line length limits are a frequent inconvenience and can make code less readable for me personally. And let me preface this by saying that I am definitely no expert when it comes to accessibility, the following is just my understanding of common accessibility guidelines. (If there are accessibility guidelines that are specific to code style, I'd be very interested in reading those.)


line length limits are a terrible way to manage complexity

@ljharb While I agree with this statement, I believe line length limits serve purposes other than managing complexity.

On a standard monitor and with default editor settings, I can open two 80-column text files next to each other without having to scroll in more than one dimension, or one 80-column text file using a much larger font size.

Most accessibility guidelines recommend limiting line lengths. For web content, the WCAG require content to be presentable such that the "width is no more than 80 characters or glyphs" and such that "text can be resized without assistive technology up to 200 percent in a way that does not require the user to scroll horizontally to read a line of text on a full-screen window". (Coincidentally, Zoom is also the first section of VS Code's Accessibility docs.)

And sure, the WCAG do not require the content to fulfill that property directly, they only require that the content can be presented in such a manner, so it is not uncommon to see the response "enable word wrap in the editor," which works quite well for text with no indentation. However, if editors (and Git and GitHub) were capable of properly re-formatting code to account for user preferences including word wrapping etc., we wouldn't have to worry about formatting in the first place.


it actually makes debugging harder. The current length is so short that lots of code is less readable because things like function calls and if conditionals need to be broken onto several lines.

@GeoffreyBooth While I personally tend to agree, I don't know if this is true in general. For example, the WAI says:

For people with some reading or vision disabilities, long lines of text can become a significant barrier. They have trouble keeping their place and following the flow of text. Having a narrow block of text makes it easier for them to continue on to the next line in a block. Lines should not exceed 80 characters or glyphs (40 if CJK), where glyphs are the element of writing in the writing system for the text.


If the goal is to allow some longer lines here and there, I personally don't think completely disabling this rule is the way to go. We already have some sensible exceptions (and the WCAG do permit such exceptions, e.g., for long URIs) and there are ESLint directives to disable the line length limit, even for entire files if necessary.

Aside from adding exceptions and disabling the rule for small parts of the codebase, we could also consider increasing the maximum line length (even though 80 seems to align with most accessibility guidelines) or we could distinguish between soft and hard line length limits.

(No objection, just my two cents.)

@ljharb
Copy link
Member

ljharb commented Jan 14, 2022

The airbnb style guide is set to a length of 100, but length checks are ignored on any line with a string, regex, or comment. In many years, I've almost never found a line warned by max-len that was actually an issue. Scrolling is fine for the very rare use case of having two windows tiled - the beginning of the line should be enough context most of the time, and you can enable soft wrapping for every other time.

@tniessen
Copy link
Member

The airbnb style guide is set to a length of 100, but length checks are ignored on any line with a string, regex, or comment.

I have no idea how much attention the airbnb style guide pays to accessibility guidelines, but either way, this seems to be closer to what I suggested in my previous comment than to disabling the max-len rule entirely.

@ljharb
Copy link
Member

ljharb commented Jan 14, 2022

I’d love to hear some justification for why it’s an accessibility issue at all - tiled windows surely isn’t about accessibility.

@tniessen
Copy link
Member

I’d love to hear some justification for why it’s an accessibility issue at all

I'm not sure I understand the question. I am literally just quoting existing accessibility guidelines, which say that text should be presentable such that the "width is no more than 80 characters or glyphs" and such that "text can be resized without assistive technology up to 200 percent in a way that does not require the user to scroll horizontally to read a line of text on a full-screen window".

@ljharb
Copy link
Member

ljharb commented Jan 14, 2022

I’m suggesting we don’t take accessibility guidelines on blind faith. I usually can quickly intuit or google the rationale behind a11y guidelines; I really don’t understand why an 80 char limit is an a11y issue.

@bnb
Copy link
Contributor

bnb commented Jan 14, 2022

@ljharb I think what @tniessen is saying is that he's already written out his perspective about why it might be an a11y issue in #41509 (comment)

@ljharb
Copy link
Member

ljharb commented Jan 14, 2022

hmm, fair enough - on a reread tho i still don’t buy it especially given the quantity of content that requires soft-wrapping and the quantity of experiences (including the entire web) that does soft-wrap, but i suppose I’m not in a position to question it.

@Trott
Copy link
Member Author

Trott commented Jan 15, 2022

Perhaps #41536 is a more modest proposal that raises fewer concerns.

Copy link
Member

@GeoffreyBooth GeoffreyBooth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer this proposal. I also see no accessibility concern. Editors have word wrap (and variable font sizes) and we’re not responsible for how GitHub’s UI handles accessibility; and there are plenty of browser extensions to customize that, as well. In my opinion, the upside of more readable code far outweighs the need for some people to use the settings available to them to wrap long lines.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think 80 is likely too low, but having no limit encourages less readable patterns.
I usually do not enforce it in my projects and I find either:

  1. very long lines being committed
  2. myself asking for lines to be split up in PRs

I would prefer we had some kind of upper limit, but I’m also ok if we don’t.

@jasnell
Copy link
Member

jasnell commented Jan 15, 2022

I'm generally happy with the length limits as they are. However, increasing to 100 would be reasonable. No length limit encourages poor style and is far less readable for me

@bricss
Copy link

bricss commented Jan 16, 2022

Cranking max-len to 120 📐 would be a perfect fit 🥋

@zekth
Copy link

zekth commented Jan 16, 2022

As @jasnell and @mcollina stated, No limit encourages poor style. I usually juggle between 80 to 120 on projects.
Regarding accessibility indeed there is a lot of tooling that allowss you to customize your DX to make a better environment; so i don't really get the a11y concerns as we're not all using the same media to read codebase.

@Trott
Copy link
Member Author

Trott commented Jan 16, 2022

encourages less readable patterns.

encourages poor style and is far less readable

People say this as if it's obviously true, but I'm skeptical. This does not align with my experience.

Does anyone have data/research on this? And if so, is it recent? (The 80-character limit is an artifact of 80-character-wide terminals. That made sense in the 1980s. No one programs on those anymore.)

In my experience, having line length limits encourages using line breaks to be added in confusing and hard-to-read ways. I think my initial skepticism came when I found confusing new RegExp() calls on strings split across multiple lines in Node.js core. These were far more understandable as regex literals (e.g., /foo/) but you can't split a regex literal across lines. We added the allowance for lines containing regex literals to extend beyond 80 characters, but problems with max-len persist.

Without a length limit, people split the lines where it helps them rather than in an effectively arbitrary place. That makes for readable code; if a line is long but contains a logical unit of information, that is more readable than splitting it across multiple lines.

@Trott
Copy link
Member Author

Trott commented Jan 16, 2022

People say this as if it's obviously true, but I'm skeptical. This does not align with my experience.

A reasonable counterpoint to what I wrote might be that, as @mcollina suggested, line length limits are good but 80 is too low. We could certainly experiment with increasing the length to 120 rather than removing limits entirely.

@targos
Copy link
Member

targos commented Jan 16, 2022

My experience is that it's the current 80-character limit that encourages less readable patterns. It's also annoying when writing code because ESLint is unable to fix line length errors, so I end up using Prettier when I don't know exactly how to write some things on multiple lines.

@jasnell
Copy link
Member

jasnell commented Jan 17, 2022

People say this as if it's obviously true, but I'm skeptical. This does not align with my experience.

That's why it's just my opinion :-) We all have different experiences on this. The current 80-character limit is too short. No limit is not great either. Let's increase the limit and go with that for a while to see how it goes. We won't really know until we try.

Assuming we do increase the limit, however, let's please discourage PRs that only change the style/formatting.

Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Making my -1 explicit. I'm not in favor of removing the limit entirely. We should first try increasing it. Let's try 120 for now and try that on for a while.

@tniessen
Copy link
Member

Alternative: #41536
Alternative: #41586

@targos
Copy link
Member

targos commented Jan 24, 2022

I suppose this can be closed now that #41586 landed?

@GeoffreyBooth
Copy link
Member

I still think the limit should be removed entirely. 120 is better than 80, but I think we can trust our contributors to know when to break.

@Trott Trott closed this Apr 6, 2022
@Trott Trott deleted the max-len-off branch September 25, 2022 17:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. needs-ci PRs that need a full CI run. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet