1247186 - Change the max-len rule in devtools/.eslintrc

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Patrick Brosset <:pbro>]

This is our current configuration:

"max-len": [1, 80, 2, {"ignoreUrls": true, "ignorePattern": "^\\s*loader\\.lazy"}],

This means that lines must be under *80* chars long otherwise a *warning* will be reported. Except for lines that contain URLs, and lines that match 'loader.lazy'.

I'm proposing that we change this to:

"max-len": [2, 100, 2, {"ignoreUrls": true, "ignorePattern": "^\\s*const\\s.+=\\s*require\\s*\\(|^\\s*loader\\.lazy"}],

This means that lines must be under *100* chars long other an *error* will be reported. Except for lines that contain URLs, and lines that match 'loaded.lazy' and lines that match 'const ... = require('.

Here's the reasoning about this:

- 80 -> 100: I've had a short discussion with 3/4 people last week on IRC about this. From a quick poll it looks like: either people don't care/don't have a fixed limit as long as things look good, or people have 100/120.
My explanation for having 80 in the first place is that 2,5 years ago, when we discussed our style guide at the Paris work week, the team agreed on 80 because it worked better for a 2 columns editor layout and small (macbook air 11'') screens.
My impression is that a lot of people do use 2 columns but tend to use larger (at least 13'') screens.

- warning -> error: see bug 1246904

- adding require to the ignorePattern: this is to avoid really ugly line wrapping 

The reason I think we should still have a rule about this, as opposed to no rule at all like we used to, is that I don't want reviewers to have to care too much about this.
In most cases, the decision of where to wrap a long line isn't really worth discussing between 2 people. So, better have a machine warn users about lines that are too long, than a human.

Comment 1

[Patrick Brosset <:pbro>]

Attachment: Bug_1247186_-_1_-_Add_an_ignorePattern_to_eslint_m.diff

Honza, could you try that with your current patch and tell me if this gets rid of nasty warnings for long require lines?

Comment 2

[Jan Honza Odvarko [:Honza] (always need-info? me)]

I am using the following code that would also be nice to ignore:

const { Tabs, TabPanel } = createFactories(require("devtools/client/jsonview/components/reps/tabs"));

(createFactories function calls React.createFactory for exposed components)

Perhaps:
^\\s*const\\s.+=\\D*require\\s*\\(|^\\s*loader\\.lazy

Honza

Comment 3

[jodvarko]

Patrick, what do you think about modifying the ignore patter a bit (see my comment #2)?

Honza

Comment 4

[Patrick Brosset <:pbro>]

Why use \D in the regex? I don't see why we would want to prevent something like `const foo = doSomething2(require("foo/bar"));
Now that I think of it, I don't think we should have const in this regex either (why would this rule care if we used var or let or this.something ...).
Why not something simpler like: \s*require\s*\( 

So we would change the ignorePattern to:

\\s*require\\s*\\(|^\\s*loader\\.lazy

Comment 5

[jodvarko]

Works for me!

Honza

Comment 6

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Please update the patch and I'll r+
Honza

Comment 7

[Patrick Brosset <:pbro>]

Attachment: Bug_1247186_-_1_-_Add_an_ignorePattern_to_eslint_m.diff

Comment 8

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8719407 [details] [diff] [review]
Bug_1247186_-_1_-_Add_an_ignorePattern_to_eslint_m.diff

Thanks!

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/ad775f52ffb9

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ad775f52ffb9

Comment 11

[Patrick Brosset <:pbro>]

Attachment: Bug_1247186_-_2_-_Set_max-len_rule_to_report_error.diff

Part 2: report errors instead of warnings. 
Turns out that I didn't need to fix anything, as all our un-ignored code already complies with this rule.

Comment 12

[Patrick Brosset <:pbro>]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7a05f6c64b8d

Comment 13

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8723620 [details] [diff] [review]
Bug_1247186_-_2_-_Set_max-len_rule_to_report_error.diff

Review of attachment 8723620 [details] [diff] [review]:
-----------------------------------------------------------------

Works for me.
Honza

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/6fe18594d84e

Comment 15

[Patrick Brosset <:pbro>]

Ok let's close this for now. The first 2 changes were obvious, but the change from 80 to 100 is less so. We'll revisit later. Also, now that require/loader/urls are excluded, I'm not sure making the lines longer really matter all that much. It would also mean that a lot of line wrapping would look weird now.

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6fe18594d84e