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

Status: RESOLVED FIXED

(Developer Infrastructure :: Lint and Formatting, enhancement)

Description

[Myk Melez [:myk] [@mykmelez]]

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)

Comment 1

[Dave Townsend [:mossop]]

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?

Comment 2

[Mark Banner (:standard8)]

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.

Comment 3

[Myk Melez [:myk] [@mykmelez]]

(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.

Comment 4

[Myk Melez [:myk] [@mykmelez]]

(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

Comment 5

[Myk Melez [:myk] [@mykmelez]]

(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.

Comment 6

[Mark Banner (: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.

Comment 7

[Myk Melez [:myk] [@mykmelez]]

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?

Comment 8

[Dave Townsend [:mossop]]

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

Comment 9

[Myk Melez [:myk] [@mykmelez]]

Attachment: prohibit blank lines at the beginning and end of blocks (eslint padded-blocks)

Comment 10

[Myk Melez [:myk] [@mykmelez]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0a3d28420931f5127363bbb0b6b805ce7c1b53a6

Comment 11

[Pulsebot]

Pushed by [e-mail]:
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

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/73a91e84dbec

Comment 13

[Pulsebot]

Pushed by [e-mail]:
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