Noissue - Fix indentation errors for ESLint 4

Noissue - Fix indentation errors for ESLint 4

[Manish Jethani, 2017-07-19 13:28:38]

Patch Set 1

I couldn't find a way to fix these errors without modifying the code or
disabling indentation checks entirely.

For the rest of the 104 errors, we have to update the config (see comments on
the issue).

https://codereview.adblockplus.org/29492577/diff/29492578/options.js
File options.js (right):

https://codereview.adblockplus.org/29492577/diff/29492578/options.js#newcode76
options.js:76: ["synchronize_invalid_url",
This is a genuine error.

[Manish Jethani, 2017-07-21 09:28:40]

https://codereview.adblockplus.org/29492577/diff/29492578/lib/devtools.js
File lib/devtools.js (right):

https://codereview.adblockplus.org/29492577/diff/29492578/lib/devtools.js#new...
lib/devtools.js:84: (
Reported a bug for this:

https://github.com/eslint/eslint/issues/8977

[Manish Jethani, 2017-07-21 09:45:17]

Patch Set 2: Target the release after 4.2.0 instead

There will be a release of ESLint today that fixes the main bug:

https://github.com/eslint/eslint/issues/8921

This minimizes the changes required to only two changes, of which one is a
genuine error that should be fixed anyway.

[Manish Jethani, 2017-07-24 11:02:58]

https://codereview.adblockplus.org/29492577/diff/29494555/options.js
File options.js (left):

https://codereview.adblockplus.org/29492577/diff/29494555/options.js#oldcode76
options.js:76: ["synchronize_invalid_url",
If 29496566 lands then we should probably still make this one change since it's
still an error, even though ESLint 4 in legacy mode ignores it.

[Sebastian Noack, 2017-08-18 10:39:21]

I agree to the changes in options.js, using 3 space indentation there seems to
have been a mistake in the first place. However, as for the changes to
lib/devtools.js, I find the original indentation there more readable. If there
is no way to make ESLint allow this kind of indentation anymore, this seems like
a regression bug in ESLint to me.

[kzar, 2017-08-18 12:11:57]

On 2017/08/18 10:39:21, Sebastian Noack wrote:
> I agree to the changes in options.js, using 3 space indentation there seems to
> have been a mistake in the first place. However, as for the changes to
> lib/devtools.js, I find the original indentation there more readable. If there
> is no way to make ESLint allow this kind of indentation anymore, this seems
like
> a regression bug in ESLint to me.

IMO we should close this review but take the examples to the discussion in #5449
about what the indentation rules should be.

[Manish Jethani, 2017-08-18 12:44:49]

On 2017/08/18 12:11:57, kzar wrote:
> On 2017/08/18 10:39:21, Sebastian Noack wrote:
> > I agree to the changes in options.js, using 3 space indentation there seems
to
> > have been a mistake in the first place. However, as for the changes to
> > lib/devtools.js, I find the original indentation there more readable. If
there
> > is no way to make ESLint allow this kind of indentation anymore, this seems
> like
> > a regression bug in ESLint to me.
> 
> IMO we should close this review but take the examples to the discussion in
#5449
> about what the indentation rules should be.

Do you think it makes sense to make the options.js change here?

[Sebastian Noack, 2017-08-18 13:30:10]

On 2017/08/18 12:44:49, Manish Jethani wrote:
> Do you think it makes sense to make the options.js change here?

As far as I understand it, the changes to options.js here, are necessary even
with the current ESLint configuration, and they are uncontroversial anyway. So I
agree that we should update this review to only do the change to options.js, and
then revisit the other changes once we move away from indent-legacy.

[Manish Jethani, 2017-08-18 13:45:18]

On 2017/08/18 13:30:10, Sebastian Noack wrote:
> On 2017/08/18 12:44:49, Manish Jethani wrote:
> > Do you think it makes sense to make the options.js change here?
> 
> As far as I understand it, the changes to options.js here, are necessary even
> with the current ESLint configuration, and they are uncontroversial anyway. So
I
> agree that we should update this review to only do the change to options.js,
and
> then revisit the other changes once we move away from indent-legacy.

The changes to options.js are not strictly necessary because legacy mode will
not complain about the incorrect indentation.

But since there's no doubt that the indentation there is incorrect, it makes
sense to fix it.

[Manish Jethani, 2017-08-18 13:45:33]

Patch Set 3: Only fix indentation in options.js

[Sebastian Noack, 2017-08-18 13:47:40]

On 2017/08/18 13:45:18, Manish Jethani wrote:
> The changes to options.js are not strictly necessary because legacy mode will
> not complain about the incorrect indentation.
> 
> But since there's no doubt that the indentation there is incorrect, it makes
> sense to fix it.

LGTM. However, in that case this review/commit should be "Noissue" rather than
referring to 5429.

[Manish Jethani, 2017-08-18 14:22:04]

On 2017/08/18 13:47:40, Sebastian Noack wrote:
> On 2017/08/18 13:45:18, Manish Jethani wrote:
> > The changes to options.js are not strictly necessary because legacy mode
will
> > not complain about the incorrect indentation.
> > 
> > But since there's no doubt that the indentation there is incorrect, it makes
> > sense to fix it.
> 
> LGTM. However, in that case this review/commit should be "Noissue" rather than
> referring to 5429.

Thanks, I've updated the title here.

[kzar, 2017-08-21 13:01:13]

LGTM

[kzar, 2017-08-21 13:01:58]

On 2017/08/21 13:01:13, kzar wrote:
> LGTM

(Assuming it still lints with "indent-legacy", otherwise I think we should wait
until switching back to "indent" to do this.)

[Manish Jethani, 2017-08-21 14:31:07]

On 2017/08/21 13:01:58, kzar wrote:
> On 2017/08/21 13:01:13, kzar wrote:
> > LGTM
> 
> (Assuming it still lints with "indent-legacy", otherwise I think we should
wait
> until switching back to "indent" to do this.)

Thanks, it does lint successfully with indent-legacy.