Opened 6 months ago

Closed 2 months ago

#7469 closed change (rejected)

eslint: disallow more than one empty line

Reported by: hfiguiere Assignee: hfiguiere
Priority: P3 Milestone:
Module: Automation Keywords:
Cc: mjethani, sebastian, kzar, tlucas, greiner Blocked By:
Blocking: Platform: Unknown / Cross platform
Ready: no Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

https://gitlab.com/eyeo/auxiliary/eyeo-coding-style/merge_requests/2

Description

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

Change History (12)

comment:1 Changed 6 months ago by tlucas

  • Priority changed from Unknown to P3
  • Ready set

comment:2 Changed 6 months ago by hfiguiere

  • Status changed from new to reviewing

comment:3 Changed 6 months ago by hfiguiere

  • Review URL(s) modified (diff)

comment:4 Changed 6 months ago by hfiguiere

  • Review URL(s) modified (diff)

comment:5 Changed 6 months ago by kzar

  • Cc kzar added; dave removed

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.

comment:6 Changed 6 months ago by 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.

comment:7 follow-up: Changed 6 months ago by sebastian

  • Ready unset

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();

comment:8 Changed 6 months ago by hfiguiere

I already have a merge request for adblockpluscore

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

Update: merged.

Last edited 6 months ago by hfiguiere (previous) (diff)

comment:9 in reply to: ↑ 7 Changed 6 months ago by 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.

comment:10 Changed 6 months ago by 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

comment:11 Changed 6 months ago by hfiguiere

  • Review URL(s) modified (diff)

Re issued the MR in the right place.

comment:12 Changed 2 months ago by hfiguiere

  • Resolution set to rejected
  • Status changed from reviewing to closed
Note: See TracTickets for help on using tickets.