Page MenuHomePhabricator

[root] [chore] [ENG-1083] update eslint config to enforce comment width to 80 characters
ClosedPublic

Authored by benschac on May 4 2022, 11:16 AM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, May 17, 9:15 AM
Unknown Object (File)
Wed, May 15, 6:13 AM
Unknown Object (File)
Mon, Apr 29, 2:53 PM
Unknown Object (File)
Apr 18 2024, 9:53 PM
Unknown Object (File)
Apr 18 2024, 9:53 PM
Unknown Object (File)
Apr 18 2024, 9:53 PM
Unknown Object (File)
Apr 18 2024, 9:53 PM
Unknown Object (File)
Apr 18 2024, 9:53 PM

Details

Summary

Use max-len rule. Code value in the config is default 80 and there's no way to turn it off. Hacking it with length of 420.

https://eslint.org/docs/rules/max-len

https://linear.app/comm/issue/ENG-1083/eslint-rule-for-80-char-width-comments

Test Plan

you should see errors when running yarn eslint

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

ashoat requested changes to this revision.May 4 2022, 11:44 AM

Discussed offline, this didn't actually do what @benschac thought it did, but he has another idea

This revision now requires changes to proceed.May 4 2022, 11:44 AM

just tested with comment, it's working as expected.

.eslintrc.json
18 ↗(On Diff #12239)

I think it might be a good idea to mention somewhere here that the 80 character width limit that we try to maintain in our codebase (because we miss typewriters or something) is handled/enforced by prettier rather than eslint.

It wasn't immediately obvious to me that setting code to 420 here wouldn't "break" the enforcement of that rule. Maybe mention that the reason we don't want to set an 80 character max-len is to avoid issues with long imports?

It may also be good to change the value from 420 to something like 9999. I think being another order of magnitude above 80 makes it clear that the intention is for the value to effectively be infinity. It would also be good to use 9s instead of a number which has negative connotations.

atul requested changes to this revision.May 4 2022, 12:28 PM
This revision now requires changes to proceed.May 4 2022, 12:28 PM
ashoat added inline comments.
.eslintrc.json
19–22 ↗(On Diff #12242)

Can you clarify that Prettier is the one formatting import statements to >80 chars? Something like:

// Prettier is configured to keep lines to 80 chars, but there are two issues:
// - It doesn't handle comments (leaves them as-is)
// - It makes all import statements take one line (reformats them)
// We want ESLint to warn us in the first case, but not in the second case
// since Prettier forces us in the second case. By setting code to 420, we
// make sure ESLint defers to Prettier for import statements.
This revision is now accepted and ready to land.May 4 2022, 1:43 PM