Auto-fix ESLint errors and warnings

it doesn't impact the compiled js files too much

Thank GOD. Then, it's reviewable :)

function _toConsumableArray(arr) { if (Array.isArray(arr)) { for (var i = 0, arr2 = Array(arr.length); i < arr.length; i++) { arr2[i] = arr[i]; } return arr2; } else { return Array.from(arr); } }

I haven't really dive into the review. Will ESLint convert the code this way? ( Provided by Airbnb? )

Any way to ask CORE Committers feedback before patch review. Not sure if they accept big patch or not.

Don't you believe or not. I scaned line-by-line :p. It's verrrrrry simple changes.

the failture on this test file:

diff --git a/core/modules/locale/tests/locale_test.js b/core/modules/locale/tests/locale_test.js

the point I mentioned on #4
https://github.com/airbnb/javascript#functions--spread-vs-apply

RTBC if you fix the test case

My first check, the patch looks okay.

These 2 commands will help you to review:

git diff --color-words=. --cached "*[^es6].js"
git diff --color-words=. --cached "*.es6.js"

I removed locale_test.es6.js altogether and left locale_test.js as it was before the rename.

Needs a follow-up to ignore it or other treatment, right?

Hmm.. make sense.

Guessing we need to file an issue soon to handle template literal. (PHP using regex to match it I bet)

Ideally, before we remove the older browser supports:
http://kangax.github.io/compat-table/es6/#test-template_literals

Now, the patch looks good. I will do my 2nd review this weekend.

After some more thoughts and review & tests. I think I will mark it as RTBC.

Needs a final rerolls.

@droplet @nod_

I just ran this against the latest HEAD, c122124367. 👍

✖ 1288 problems (528 errors, 760 warnings)

- Minor issue with escaped strings.

Thanks for the reroll @drpal, so I can review it again :)

core/modules/locale/tests/locale_test.es6.js

We have to ignore this file completely to let the tests to catch the SINGLE / DOUBLE QUOTE bugs. (@see #8)

@droplet

Ah yes. Thanks for pointing that out. 👍

Sorry; I should have run that test locally before uploading. 🤔

🚀🚀🚀🚀🚀🚀

- rerolled at 950362eb48.

Missing the JS files

@droplet 👍

🚀🚀🚀🚀🚀🚀

Make life easier

.\node_modules\.bin\eslint.cmd --fix . || true && yarn build:js

git rm -f modules/locale/tests/locale_test.es6.js && git checkout 9a0e9a649a -- modules/locale/tests/locale_test.js && git add -u && git diff --cached > auto_fix_eslint_errors-2880007.patch

Maybe this time? 🙅

Our pre-commit hooks would actually prevent committing this so I suggest we try to find another solution for #8.

AFAIK, Babel has no ignores syntax. Let's make the pre-commit hooks compatible with CORE. It's not worth to change the build script or introduce a pattern to our developers to handle this one exception. and may be the only one forever.

Couldn't we add an eslint ignore to that file, so that the auto fix wouldn't make changes to the file?

I wish we can. The change comes from Babel rather than ESLint fix. To check out the unpatched you will have a clear overview :p

Thanks @droplet. I was first only looking at the failing tests, and noticed that the only test breaking changes come from eslint. Now that I dug a bit deeper it looks like we don't want babel to compile this since this it needed for the locale module regex tests. It seems like not having .es6 version of this file is the accurate thing to do.

I think we should add a comment to the file why this is the case since at least for me this wasn't obvious at first. Probably updating the file documentation to point out to the correct file would be a good start.

In the meanwhile, I will try to figure out what would be the best way to make the git hooks support this exception.

OK. I leave it for someone able to write a better comment :p

This line blocking the commit I think:
https://github.com/alexpott/d8githooks/blob/master/pre-commit-8-4#L127

Also, maybe we should do it (#37) in a new issue thread and don't mess with the big patch together. It's easier to review/rerolls.

EDIT:

OK. I've done it: #2889600: [regression] Restore \LocaleJavascriptTranslationTest test coverage and keep testing processed JS file

I think we able to skip the local_test in this patch. Auto-fix ESLint doesn't fix all issues.

I'm postponing this until we get a resolution on #2889600: [regression] Restore \LocaleJavascriptTranslationTest test coverage and keep testing processed JS file.

Why not commit it straightly? I removed the local_test change in #40.

ESLint on d.org is broken now, I think we will have 2 round or 3 autofix.

This issue will unlock manual code refactoring to ES6 standard I think. (Pretty a lot of work still)

@droplet

Yep. This is blocking all progress on the manual code refactoring. I didn't notice that you'd removed the changes in #40, it's pretty hard to review this huge patch. We can figure out how to handle the locale_test.js regressions later.

I did a couple of things to review the patch:

• Run ./node_modules/.bin/eslint --fix . || true && yarn build:js and confirmed that only differences were that the patch from #40 has reverted changes to local_test. I agree that we should handle that in separated issue.
• Skimmed through the changes to better understand how eslint has fixed the coding standard failures. One of the things that caught my attention was the removal of use strict which I think is fine after reading this Github issue.
• Applied the patch and confirmed that at least few of the most popular pages render correctly since the lack of JS test coverage.

Committed 612c1fa and pushed to 8.4.x. Thanks!