Closed Bug 1247186 Opened 4 years ago Closed 4 years ago

Change the max-len rule in devtools/.eslintrc


(DevTools :: General, defect)

Not set


(firefox47 fixed)

Firefox 47
Tracking Status
firefox47 --- fixed


(Reporter: pbro, Assigned: pbro)



(2 files, 1 obsolete file)

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.
Honza, could you try that with your current patch and tell me if this gets rid of nasty warnings for long require lines?
Assignee: nobody → pbrosset
Attachment #8717916 - Flags: review?(odvarko)
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)


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

Flags: needinfo?(pbrosset)
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:

Flags: needinfo?(pbrosset)
Works for me!

Please update the patch and I'll r+
Flags: needinfo?(pbrosset)
Attachment #8717916 - Attachment is obsolete: true
Attachment #8717916 - Flags: review?(odvarko)
Flags: needinfo?(pbrosset)
Attachment #8719407 - Flags: review?(odvarko)
Comment on attachment 8719407 [details] [diff] [review]

Attachment #8719407 - Flags: review?(odvarko) → review+
Keywords: leave-open
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.
Attachment #8723620 - Flags: review?(odvarko)
Comment on attachment 8723620 [details] [diff] [review]

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

Works for me.
Attachment #8723620 - Flags: review?(odvarko) → review+
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.
Keywords: leave-open
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.