Closed Bug 1247186 Opened 4 years ago Closed 4 years ago

Change the max-len rule in devtools/.eslintrc

Categories

(DevTools :: General, defect)

defect
Not set

Tracking

(firefox47 fixed)

RESOLVED FIXED
Firefox 47
Tracking Status
firefox47 --- fixed

People

(Reporter: pbro, Assigned: pbro)

Details

Attachments

(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
Status: NEW → ASSIGNED
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)

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

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

Honza
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:

\\s*require\\s*\\(|^\\s*loader\\.lazy
Flags: needinfo?(pbrosset)
Works for me!

Honza
Please update the patch and I'll r+
Honza
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]
Bug_1247186_-_1_-_Add_an_ignorePattern_to_eslint_m.diff

Thanks!
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]
Bug_1247186_-_2_-_Set_max-len_rule_to_report_error.diff

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

Works for me.
Honza
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
https://hg.mozilla.org/mozilla-central/rev/6fe18594d84e
Status: ASSIGNED → RESOLVED
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.