apply eslint rule "padded-blocks": ["error", "never"] to entire tree

RESOLVED FIXED in Firefox 67

Status

enhancement
RESOLVED FIXED
3 months ago
3 months ago

People

(Reporter: myk, Assigned: myk)

Tracking

unspecified
mozilla67

Firefox Tracking Flags

(firefox67 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

3 months ago

A variety of subdirectories apply the eslint rule "padded-blocks": ["error", "never"] to prevent blank lines at the beginning and end of blocks [1], and only one subdirectory turns off that rule (devtools/client/dom/); so it seems like the kind of rule we would benefit from applying tree-wide.

[1] https://searchfox.org/mozilla-central/search?q=padded-blocks

Doing so would require us to fix up a bunch of instances by removing blank lines. In this particular case, however, the changes wouldn't clutter blame views, since lines would only be removed.

mossop: applying this rule would require removing many such lines from toolkit/ and browser/; do you have any objection to applying this rule to those directories (or to the tree more generally)?

./mach eslint toolkit/
✖ 317 problems (317 errors, 0 warnings)
./mach eslint browser/
✖ 206 problems (206 errors, 0 warnings)

Flags: needinfo?(dtownsend)

Generally I expect I'd support this, though I'd like to look over a few examples of code that currently uses this. Do you happen to have some examples handy?

Component: General → Lint and Formatting
Flags: needinfo?(dtownsend)
Product: Toolkit → Firefox Build System

FWIW I'm for this as well. We should just do this tree-wide as I'm pushing for rule harmonization across the tree.

If we want to go ahead, might be worth assessing the impact on various areas and if we need to do a comma-dangle style roll-out in a soft freeze quiet period near a merge point, or just roll it out at any time.

(Assignee)

Comment 3

3 months ago

(In reply to Dave Townsend [:mossop] (he/him) from comment #1)

Generally I expect I'd support this, though I'd like to look over a few examples of code that currently uses this. Do you happen to have some examples handy?

Here's what it looks like to add the rule and then fix all the occurrences in browser/ and toolkit/:

https://github.com/mozilla/gecko/compare/central...mykmelez:fix-padded-blocks

Most of those were fixed automatically with mach eslint --fix, although the occurrences in the XML files had to be fixed manually.

(Assignee)

Comment 4

3 months ago

(In reply to Dave Townsend [:mossop] (he/him) from comment #1)

Generally I expect I'd support this, though I'd like to look over a few examples of code that currently uses this. Do you happen to have some examples handy?

Erm, if you were asking about code that already applies that rule, the set of browser/ and toolkit/ subdirectories that currently apply it is:

browser/components/migration/
browser/components/newtab/
browser/components/payments/
browser/extensions/formautofill/
browser/extensions/webcompat-reporter/
toolkit/components/extensions/
toolkit/components/narrate/
toolkit/components/satchel/

However, the code in these subdirectories doesn't look that different from the rest of browser/ and toolkit/, since most of that code also uses the style. The several hundred exceptions are a small fraction of the total number of blocks in the rest of browser/ and toolkit/, most of which also aren't padded.

See the padded-blocks documentation for more succinct examples of padded vs. non-padded blocks:

https://eslint.org/docs/rules/padded-blocks

(Assignee)

Comment 5

3 months ago

(In reply to Mark Banner (:standard8) from comment #2)

FWIW I'm for this as well. We should just do this tree-wide as I'm pushing for rule harmonization across the tree.

In that blog post, you mention other rules that you'd like to apply across the tree in the "Harmonize our rules" section. Would you like to harmonize rules in one fell swoop (f.e. to limit the number of times we break patches in progress), or would you prefer to harmonize them one rule at a time, as this bug suggests for padded-blocks (f.e. to be able to make progress incrementally).

If we want to go ahead, might be worth assessing the impact on various areas and if we need to do a comma-dangle style roll-out in a soft freeze quiet period near a merge point, or just roll it out at any time.

Fixing browser/ and toolkit/ currently touches 316 JS source files (plus adds the rule to tools/lint/eslint/eslint-plugin-mozilla/lib/configs/recommended.js and removes it from eight directory-specific .eslintrc.js files). But very few lines are touched in each file.

For example, gecko/browser/actors/AboutReaderChild.jsm is 152 lines, and the fix removes a single line from it; while toolkit/components/aboutmemory/content/aboutMemory.js is 2098 lines, and the fix removes seven lines from it.

That latter file was one of five that had conflicts when I just re-merged central into the branch I had created two days ago to test this change, however. So I expect any patch for this to become out-of-date quickly and to break at least some other in-flight patches when it lands.

Flags: needinfo?(standard8)

(In reply to Myk Melez [:myk] [@mykmelez] from comment #5)

(In reply to Mark Banner (:standard8) from comment #2)

FWIW I'm for this as well. We should just do this tree-wide as I'm pushing for rule harmonization across the tree.

In that blog post, you mention other rules that you'd like to apply across the tree in the "Harmonize our rules" section. Would you like to harmonize rules in one fell swoop (f.e. to limit the number of times we break patches in progress), or would you prefer to harmonize them one rule at a time, as this bug suggests for padded-blocks (f.e. to be able to make progress incrementally).

I haven't looked at them in all that detail, but I suspect there's quite a few to sort out, and they may be a bit more contentious across the tree (though we'll see). What I'm also hoping that I didn't write there is that we might get something like prettier (sometime) which would then just make the style rules go away (but in the meantime, I'm kinda handling them on a "as demand requires" basis).

It might makes sense to batch them up a bit, though I think this one could be done on its own. At the moment, I'm mainly concentrating on getting more directories enabled (I have a set of mentored bugs in progress from the end of last year), and adding new rules sometimes complicates that work a bit.

That latter file was one of five that had conflicts when I just re-merged central into the branch I had created two days ago to test this change, however. So I expect any patch for this to become out-of-date quickly and to break at least some other in-flight patches when it lands.

I think with the js files we're unlikely to not break someone. So we should just go for it when we want. It sounds like this isn't massive, so maybe we can just land this when we're ready to.

Flags: needinfo?(standard8)
Severity: normal → enhancement
(Assignee)

Comment 7

3 months ago

Here's what it looks like to do this across the whole tree:

https://github.com/mozilla/gecko/compare/central...mykmelez:fix-padded-blocks-whole-tree

mossop: Should I move forward with this and create a patch; if so, should it be for the whole tree; and if so, is there anyone else whose approval/review I should obtain for it besides yours and standard8's?

Flags: needinfo?(dtownsend)

Yes I think we should just go ahead with this. I'm happy to review the tree-wide changes.

Flags: needinfo?(dtownsend)
(Assignee)

Updated

3 months ago
Assignee: nobody → myk
Status: NEW → ASSIGNED
(Assignee)

Comment 10

3 months ago
Summary: consider applying eslint rule "padded-blocks": ["error", "never"] to entire tree → apply eslint rule "padded-blocks": ["error", "never"] to entire tree

Comment 11

3 months ago
Pushed by myk@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/73a91e84dbec
prohibit blank lines at the beginning and end of blocks (eslint padded-blocks) r=mossop,Standard8

Comment 12

3 months ago
bugherder
Status: ASSIGNED → RESOLVED
Last Resolved: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67

Comment 13

3 months ago
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/cf9289be51a6
Port bug 1518283 - prohibit blank lines at the beginning and end of blocks (eslint padded-blocks). rs=bustage-fix
You need to log in before you can comment on or make changes to this bug.