Opened on 02/06/2017 at 06:08:19 AM
Closed on 03/31/2017 at 09:13:50 AM
Last modified on 07/06/2017 at 01:14:05 PM
#4864 closed change (fixed)
Start using ESLint for adblockpluschrome repository
| Reported by: | kzar | Assignee: | kzar |
|---|---|---|---|
| Priority: | P2 | Milestone: | Adblock-Plus-1.13.3-for-Chrome-Opera |
| Module: | Platform | Keywords: | |
| Cc: | sebastian, Ross, rraceanu, philll, sclow | Blocked By: | #3692 |
| Blocking: | #5080 | Platform: | Unknown / Cross platform |
| Ready: | yes | Confidential: | no |
| Tester: | Ross | Verified working: | yes |
| Review URL(s): | |||
Description (last modified by kzar)
Background
We've created an ESLint configuration for the company eslint-config-eyeo (see #3692). Now we need to make changes to adblockpluschrome to use ESLint and that configuration.
What to change
- Add a ESLint configuration to adblockpluschrome which inherits from eslint-config-eyeo:
{
"extends": "eslint-config-eyeo",
"root": true,
...
}
- Add an ESLint ignore file to ignore files such as jQuery.
- Make the necessary changes to the adblockpluschrome code so that linting passes.
- Add a note to the README explaining how to lint the code. (eslint *.js lib/ qunit/ ext/ chrome/)
Hints for testers
Unfortunately these changes were pretty comprehensive and will require very thorough testing of pretty much all code paths. Please take some time to test this on Chrome 49, a recent version such as 57 and Opera. I recommend testing this along side #5061 (and perhaps #5023) since that also contained large sweeping changes. Really anything and everything you can think of needs to be tested here.
Attachments (0)
Change History (16)
comment:1 Changed on 02/08/2017 at 06:36:03 AM by kzar
- Blocked By 4871 added
comment:3 Changed on 02/08/2017 at 09:04:51 AM by kzar
- Blocked By 4871 removed
- Description modified (diff)
comment:4 Changed on 03/30/2017 at 06:36:25 AM by kzar
- Blocked By 5060 added
comment:5 Changed on 03/31/2017 at 05:52:03 AM by kzar
- Blocked By 5060 removed
comment:6 Changed on 03/31/2017 at 05:59:53 AM by kzar
- Blocked By 5077 added
comment:7 Changed on 03/31/2017 at 07:34:30 AM by kzar
- Blocked By 5079 added
comment:8 Changed on 03/31/2017 at 08:46:29 AM by abpbot
comment:9 Changed on 03/31/2017 at 08:47:52 AM by kzar
- Milestone set to Adblock-Plus-for-Chrome-Opera-next
comment:10 Changed on 03/31/2017 at 09:01:01 AM by kzar
- Blocked By 5079 removed
comment:11 Changed on 03/31/2017 at 09:01:50 AM by kzar
- Blocked By 5077 removed
comment:12 Changed on 03/31/2017 at 09:13:50 AM by kzar
- Cc Ross rraceanu philll sclow added
- Description modified (diff)
- Resolution set to fixed
- Review URL(s) modified (diff)
- Status changed from new to closed
I've opened #5080 for the changes required to reduce the /* globals ... */ comments. Originally I was going to reuse this one but I realised that was probably a mistake.
Phill / Ross / Robert / Scott - Heads up testing this, #5061 and #5023 will require a whole bunch of work. I recommend combining testing of those three and starting soon as possible. There is a high likelihood of regressions with all these changes, we don't want to miss them or find them just before the next release.
comment:13 Changed on 03/31/2017 at 09:20:17 AM by sebastian
As for testing, note that the devbuilds on the Chrome Web Store don't get updated at the moment due to an issue on their end we are waiting for to get fixed, see ticket:5027#comment:25.
comment:14 Changed on 03/31/2017 at 11:49:02 AM by kzar
- Blocking 5080 added
comment:15 Changed on 04/03/2017 at 08:00:37 AM by Ross
kzar / sebastian: Have picked this up.
comment:16 Changed on 07/06/2017 at 01:14:05 PM by Ross
- Tester changed from Unknown to Ross
- Verified working set
Done. The build appears to working as expected across a variety of browser versions.
ABP 1.13.2.1785
Chrome 49 / 54 / 58 / Windows 7
Opera 36 / 42 / 45 / Windows 7
A commit referencing this issue has landed:
Issue 4864 - Start using ESLint for adblockpluschrome