Issue 29376565: Issue 3692 - Enable the no-unused-vars ESLint rule (Closed)
|
Created:
Feb. 21, 2017, 5:02 a.m. by kzar Modified:
Feb. 23, 2017, 8:34 a.m. CC:
Felix Dahlke Visibility:
Public. |
DescriptionIssue 3692 - Enable the no-unused-vars ESLint rule
See https://issues.adblockplus.org/ticket/3692#comment:35 onwards.
Patch Set 1 #
Total comments: 5
MessagesTotal messages: 10
Patch Set 1
Patch Set 1
https://codereview.adblockplus.org/29376565/diff/29376566/eslint-config-eyeo/... File eslint-config-eyeo/package.json (right): https://codereview.adblockplus.org/29376565/diff/29376566/eslint-config-eyeo/... eslint-config-eyeo/package.json:3: "version": "1.0.1", Do we even have this package already published on npmjs? Otherwise, we don't need to bump the version.
LGTM
https://codereview.adblockplus.org/29376565/diff/29376566/eslint-config-eyeo/... File eslint-config-eyeo/package.json (right): https://codereview.adblockplus.org/29376565/diff/29376566/eslint-config-eyeo/... eslint-config-eyeo/package.json:3: "version": "1.0.1", On 2017/02/21 11:16:20, Sebastian Noack wrote: > Do we even have this package already published on npmjs? Otherwise, we don't > need to bump the version. Yea I did.
https://codereview.adblockplus.org/29376565/diff/29376566/eslint-config-eyeo/... File eslint-config-eyeo/package.json (right): https://codereview.adblockplus.org/29376565/diff/29376566/eslint-config-eyeo/... eslint-config-eyeo/package.json:3: "version": "1.0.1", On 2017/02/21 11:31:03, kzar wrote: > On 2017/02/21 11:16:20, Sebastian Noack wrote: > > Do we even have this package already published on npmjs? Otherwise, we don't > > need to bump the version. > > Yea I did. I see. It just seems a little inconsistent that we have 4 changes, to eslint-config-eyeo now, while only one of them bumps the version number. Also, considering that there is no separate issue for these changes, it seems cleanest to just combine them into one patch.
https://codereview.adblockplus.org/29376565/diff/29376566/eslint-config-eyeo/... File eslint-config-eyeo/package.json (right): https://codereview.adblockplus.org/29376565/diff/29376566/eslint-config-eyeo/... eslint-config-eyeo/package.json:3: "version": "1.0.1", On 2017/02/21 11:33:53, Sebastian Noack wrote: > On 2017/02/21 11:31:03, kzar wrote: > > On 2017/02/21 11:16:20, Sebastian Noack wrote: > > > Do we even have this package already published on npmjs? Otherwise, we don't > > > need to bump the version. > > > > Yea I did. > > I see. It just seems a little inconsistent that we have 4 changes, to > eslint-config-eyeo now, while only one of them bumps the version number. Also, > considering that there is no separate issue for these changes, it seems cleanest > to just combine them into one patch. Yea, I plan to combine all four into one patch. When I created this review I didn't expect to create the other three shortly afterwards.
https://codereview.adblockplus.org/29376565/diff/29376566/eslint-config-eyeo/... File eslint-config-eyeo/package.json (right): https://codereview.adblockplus.org/29376565/diff/29376566/eslint-config-eyeo/... eslint-config-eyeo/package.json:3: "version": "1.0.1", On 2017/02/21 11:42:41, kzar wrote: > On 2017/02/21 11:33:53, Sebastian Noack wrote: > > On 2017/02/21 11:31:03, kzar wrote: > > > On 2017/02/21 11:16:20, Sebastian Noack wrote: > > > > Do we even have this package already published on npmjs? Otherwise, we > don't > > > > need to bump the version. > > > > > > Yea I did. > > > > I see. It just seems a little inconsistent that we have 4 changes, to > > eslint-config-eyeo now, while only one of them bumps the version number. Also, > > considering that there is no separate issue for these changes, it seems > cleanest > > to just combine them into one patch. > > Yea, I plan to combine all four into one patch. When I created this review I > didn't expect to create the other three shortly afterwards. LGTM, if all 4 changes will be combined into one commit.
On 2017/02/21 11:53:54, Sebastian Noack wrote: > LGTM, if all 4 changes will be combined into one commit. Frankly, I disagree. These four changes all had a separate review with separate discussion and justification. They should not be combined.
On 2017/02/21 11:55:28, Wladimir Palant wrote: > On 2017/02/21 11:53:54, Sebastian Noack wrote: > > LGTM, if all 4 changes will be combined into one commit. > > Frankly, I disagree. These four changes all had a separate review with separate > discussion and justification. They should not be combined. Whatever. It's not important enough to argue about. Still LGTM. 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
