eslint: disallow more than one empty line

[hfiguiere]

Background

We want to disallow more than one empty line. We can get eslint enforced.

What to change

Add "no-multiple-empty-lines": ["error", {"max": 1}] to eslint-config-eyeo/index.js

[kzar]

I wonder why you want to disallow multiple empty lines? Have they been a problem?

I think they occasionally can help to group code together and therefore make the code easier to read. For example, take composer.postload.js, that code is quite complicated. I spent quite a lot of time, figuring out how it worked, untangling it, fixing bugs and adding comments. There's different "sections" to that code, which we've delimited with "/* */" style comments. After each section, we added two newlines instead of one to visually separate the the two sections. In my opinion, if we enforced the no-multiple-empty-lines rule as proposed on that file it would become harder to read.

Perhaps a compromise could be to set the maximum to 2 instead? From the top of my head, I can't see when more than two would be useful.

[hfiguiere]

I have seen many reviews nit picking on that, so I was thinking it would be wiser to just enforce, but then if you think it is too much, I'm fine with it.

[sebastian]

I ran eslint --fix with the suggested rule added on adblockpluscore and adblockpluscrome, see the resulting diffs below. I think in most instances the result is an improvement, with the exception of the case Dave pointed out where you split a script in different sections, adding two blank lines over the title of the section. Then again, one could argue that you should rather split up the the script into multiple scripts than dividing it into sections. Also if it's just that one scenario, maybe it's not worth an exception to the rule, keeping us from automatically enforcing that rule everywhere else.

Anyway, let's see what others say. Removing the ready flag for now.

adblockpluscore:

diff -r b012d2682a48 test/matcher.js
--- a/test/matcher.js	Thu Apr 18 08:12:13 2019 +0200
+++ b/test/matcher.js	Thu Apr 18 17:47:33 2019 -0400
@@ -380,7 +380,6 @@
   test.ok(!matcher.matchesAny(parseURL("https://example.com/foo/bar/ad.jpg"),
                               RegExpFilter.typeMap.IMAGE));
 
-
   // Map { "example" => { text: "||example.com^$~third-party" } }
   matcher.add(Filter.fromText("||example.com^$~third-party"));
 
diff -r b012d2682a48 test/regexpFilters_matching.js
--- a/test/regexpFilters_matching.js	Thu Apr 18 08:12:13 2019 +0200
+++ b/test/regexpFilters_matching.js	Thu Apr 18 17:47:33 2019 -0400
@@ -34,8 +34,6 @@
   callback();
 };
 
-
-
 function testMatch(test, text, location, contentType, docDomain, thirdParty, sitekey, expected)
 {
   if (thirdParty && docDomain == null)
diff -r b012d2682a48 test/runners/chromium_download.js
--- a/test/runners/chromium_download.js	Thu Apr 18 08:12:13 2019 +0200
+++ b/test/runners/chromium_download.js	Thu Apr 18 17:47:33 2019 -0400
@@ -62,7 +62,6 @@
   let chromiumDir = null;
   let snapshotsDir = path.join(snapshotsRootDir(), "chromium-snapshots");
 
-
   while (true)
   {
     chromiumDir = path.join(snapshotsDir,

adblockpluschrome:

diff -r 90f9ca6487e3 build/buildlist.js
--- a/build/buildlist.js	Fri Apr 12 14:24:59 2019 -0400
+++ b/build/buildlist.js	Thu Apr 18 17:48:26 2019 -0400
@@ -27,7 +27,6 @@
 const FILENAME = "build_list.json";
 const MAXKEPTBUILDS = 150;
 
-
 /**
  * Shift a new build into the list of builds and drop the oldest one. Also
  * remove the actual file of the oldest build.
diff -r 90f9ca6487e3 build/target/gecko.js
--- a/build/target/gecko.js	Fri Apr 12 14:24:59 2019 -0400
+++ b/build/target/gecko.js	Thu Apr 18 17:48:26 2019 -0400
@@ -52,7 +52,6 @@
   );
 };
 
-
 function renderBuildList(name, urlChangelog, buildList, target)
 {
   let targetFileName = path.join(target, "index.html");
diff -r 90f9ca6487e3 composer.postload.js
--- a/composer.postload.js	Fri Apr 12 14:24:59 2019 -0400
+++ b/composer.postload.js	Thu Apr 18 17:48:26 2019 -0400
@@ -42,7 +42,6 @@
 let lastRightClickEvent = null;
 let lastRightClickEventIsMostRecent = false;
 
-
 /* Utilities */
 
 function getFiltersForElement(element, callback)
@@ -118,7 +117,6 @@
   callback(null);
 }
 
-
 /* Element highlighting */
 
 // Adds an overlay to an element in order to highlight it.
@@ -285,7 +283,6 @@
   }
 }
 
-
 /* Input event handlers */
 
 function stopEventPropagation(event)
@@ -342,7 +339,6 @@
   }
 }
 
-
 /* Element selection */
 
 // Start highlighting elements yellow as the mouse moves over them, when one is
@@ -436,7 +432,6 @@
   document.removeEventListener("keydown", keyDown, true);
 }
 
-
 /* Core logic */
 
 // We're done with the block element feature for now, tidy everything up.
diff -r 90f9ca6487e3 ext/background.js
--- a/ext/background.js	Fri Apr 12 14:24:59 2019 -0400
+++ b/ext/background.js	Thu Apr 18 17:48:26 2019 -0400
@@ -66,7 +66,6 @@
       pageMap._delete(pageId);
   }
 
-
   /* Pages */
 
   let Page = ext.Page = function(tab)
@@ -321,7 +320,6 @@
     ext.pages.onActivated._dispatch(new Page({id: details.tabId}));
   });
 
-
   /* Browser actions */
 
   let BrowserAction = function(tabId)
@@ -441,7 +439,6 @@
     }
   };
 
-
   /* Web requests */
 
   let framesOfTabs = new Map();
@@ -485,7 +482,6 @@
     });
   });
 
-
   /* Message passing */
 
   browser.runtime.onMessage.addListener((message, rawSender, sendResponse) =>
diff -r 90f9ca6487e3 ext/common.js
--- a/ext/common.js	Fri Apr 12 14:24:59 2019 -0400
+++ b/ext/common.js	Thu Apr 18 17:48:26 2019 -0400
@@ -44,7 +44,6 @@
     }
   };
 
-
   /* Message passing */
 
   ext.onMessage = new ext._EventTarget();
diff -r 90f9ca6487e3 lib/filterComposer.js
--- a/lib/filterComposer.js	Fri Apr 12 14:24:59 2019 -0400
+++ b/lib/filterComposer.js	Thu Apr 18 17:48:26 2019 -0400
@@ -279,7 +279,6 @@
   ).catch(() => {});
 });
 
-
 /* Context menu and popup button */
 
 let readyActivePages = new ext.PageMap();
diff -r 90f9ca6487e3 lib/ioIndexedDB.js
--- a/lib/ioIndexedDB.js	Fri Apr 12 14:24:59 2019 -0400
+++ b/lib/ioIndexedDB.js	Thu Apr 18 17:48:26 2019 -0400
@@ -17,7 +17,6 @@
 
 "use strict";
 
-
 const dbName = "adblockplus";
 const storeName = "file";
 const keyPath = "fileName";
diff -r 90f9ca6487e3 qunit/tests/subscriptionInit.js
--- a/qunit/tests/subscriptionInit.js	Fri Apr 12 14:24:59 2019 -0400
+++ b/qunit/tests/subscriptionInit.js	Thu Apr 18 17:48:26 2019 -0400
@@ -29,7 +29,6 @@
     }
   });
 
-
   test("Choosing filter subscriptions", assert =>
   {
     let done = assert.async();

[hfiguiere]

I already have a merge request for adblockpluscore

​https://gitlab.com/eyeo/adblockplus/adblockpluscore/merge_requests/54

Update: merged.

[greiner]

Replying to sebastian:

Anyway, let's see what others say.

Enforcing this rule seems a bit drastic because it's not solving a problem with our code. On the other hand, adding it seems harmless so if it helps speed-up reviews so I'd be fine with adding it unless we encounter any issues with it. At least looking at the provided diff, there don't appear to be any at this point.

[tlucas]

I don't have a strong opinion here, i only have the impression that this was already implicitly enforced in reviews (at least i never stumbled upon leaving 2 blank lines).

Please note, the upstream repository for this is now available at ​https://gitlab.com/eyeo/auxiliary/eyeo-coding-style

[hfiguiere]

Re issued the MR in the right place.