New version of estlint, running it on our code makes it fail.

Before patch

$ eslint .

core/misc/ajax.js
  103:14  error  Empty block statement  no-empty
  111:14  error  Empty block statement  no-empty

✖ 2 problems (2 errors, 0 warnings)

After patch, no errors.

New rule

Introduction of the new rule "one-var": [2, "never"],

This will enforce our coding standards. Our standards are a bit fuzzy and I've found several violations in core code. For that reason I'm making the standards tighter so that we can enforce them with eslint. Once this patch is RTBC I'll be editing the documentaion page. Currently we allow declaring several variables on one line if there is no assignment: var i, j, tmpStuff; this is now forbidden and will need to be written on 3 separate lines. Any good minifier will concatenate all that so there is no perf impact when minified.

Increment consistency

Not tied to ESlint but I changed the 4 increments using this style i += 1 to i++ in for loops for consistency (the new rule made it so the loops in question had to be modified anyway).

CommentFileSizeAuthor
#19 eslint_0_18_0-2461531-19.patch38.32 KBlauriii
#16 interdiff.txt195 bytesnod_
#16 core-js-eslint-rules-0.18.0-2461531-16.patch38.31 KBnod_
#7 core-js-eslint-rules-0.18.0-2461531-7.patch38.08 KBnod_
core-js-eslint-rules-0.18.0.patch38.07 KBnod_

Comments

nod_ queued core-js-eslint-rules-0.18.0.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, core-js-eslint-rules-0.18.0.patch, failed testing.

Status: Needs work » Needs review

nod_ queued core-js-eslint-rules-0.18.0.patch for re-testing.

nod_ queued core-js-eslint-rules-0.18.0.patch for re-testing.

nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Novice

RTBC because eslint did the review by itself :-°

This can prevent things like #1751436-20: Selectors clean-up: bartik/color theme.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Needs review

Would still be good to get another (human) review.

nod_’s picture

nod_
Primary language French
Location Lille
commented
StatusFileSize
new core-js-eslint-rules-0.18.0-2461531-7.patch38.08 KB

Reroll because of the is-active class name change.

Status: Needs review » Needs work

The last submitted patch, 7: core-js-eslint-rules-0.18.0-2461531-7.patch, failed testing.

nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Needs work » Needs review

well, yeah.

pguillard’s picture

pguillard commented

After careful reading (with my eyes!), I did not detect anything.
+1 for RTBC

pguillard’s picture

pguillard commented
Status: Needs review » Reviewed & tested by the community

webchick queued 7: core-js-eslint-rules-0.18.0-2461531-7.patch for re-testing.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 7: core-js-eslint-rules-0.18.0-2461531-7.patch, failed testing.

Status: Needs work » Needs review

nod_ queued 7: core-js-eslint-rules-0.18.0-2461531-7.patch for re-testing.

nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Needs review » Reviewed & tested by the community
nod_’s picture

nod_
Primary language French
Location Lille
commented
StatusFileSize
new core-js-eslint-rules-0.18.0-2461531-16.patch38.31 KB
new interdiff.txt195 bytes

the sourcemap issues added some test javascript that need to be excluded from eslint scan. Added a line in eslintingnore.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented

RTBC+1, fixes the eslint errors

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Also we have some new eslint fails...

Testing JavaScript using eslint


core/tests/Drupal/Tests/Core/Asset/js_test_files/source_mapping_url.min.js
  1:0   error  Expected an assignment or function call and instead saw an expression  no-unused-expressions
  1:1   error  Wrapping non-IIFE function literals in parens is unnecessary           no-wrap-func
  1:31  error  Missing semicolon                                                      semi

core/tests/Drupal/Tests/Core/Asset/js_test_files/source_mapping_url.min.js.optimized.js
  1:0   error  Expected an assignment or function call and instead saw an expression  no-unused-expressions
  1:1   error  Wrapping non-IIFE function literals in parens is unnecessary           no-wrap-func
  1:31  error  Missing semicolon                                                      semi

core/tests/Drupal/Tests/Core/Asset/js_test_files/source_mapping_url_old.min.js
  1:0   error  Expected an assignment or function call and instead saw an expression  no-unused-expressions
  1:1   error  Wrapping non-IIFE function literals in parens is unnecessary           no-wrap-func
  1:31  error  Missing semicolon                                                      semi

core/tests/Drupal/Tests/Core/Asset/js_test_files/source_mapping_url_old.min.js.optimized.js
  1:0   error  Expected an assignment or function call and instead saw an expression  no-unused-expressions
  1:1   error  Wrapping non-IIFE function literals in parens is unnecessary           no-wrap-func
  1:31  error  Missing semicolon                                                      semi

core/tests/Drupal/Tests/Core/Asset/js_test_files/source_url.min.js
  1:0   error  Expected an assignment or function call and instead saw an expression  no-unused-expressions
  1:1   error  Wrapping non-IIFE function literals in parens is unnecessary           no-wrap-func
  1:31  error  Missing semicolon                                                      semi

core/tests/Drupal/Tests/Core/Asset/js_test_files/source_url.min.js.optimized.js
  1:0   error  Expected an assignment or function call and instead saw an expression  no-unused-expressions
  1:1   error  Wrapping non-IIFE function literals in parens is unnecessary           no-wrap-func
  1:31  error  Missing semicolon                                                      semi

core/tests/Drupal/Tests/Core/Asset/js_test_files/source_url_old.min.js
  1:0   error  Expected an assignment or function call and instead saw an expression  no-unused-expressions
  1:1   error  Wrapping non-IIFE function literals in parens is unnecessary           no-wrap-func
  1:31  error  Missing semicolon                                                      semi

core/tests/Drupal/Tests/Core/Asset/js_test_files/source_url_old.min.js.optimized.js
  1:0   error  Expected an assignment or function call and instead saw an expression  no-unused-expressions
  1:1   error  Wrapping non-IIFE function literals in parens is unnecessary           no-wrap-func
  1:31  error  Missing semicolon
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Status: Needs work » Needs review
Issue tags: -Needs reroll
StatusFileSize
new eslint_0_18_0-2461531-19.patch38.32 KB

I can't reproduce the eslint warnings after the patch.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

@lauriii yep this fixes #18

nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Needs review » Reviewed & tested by the community

Reroll needed because of translation ui.
Applies and work.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed 49a1b84 and pushed to 8.0.x. Thanks!

  • alexpott committed 49a1b84 on 8.0.x 
    Issue #2461531 by nod_, lauriii, pguillard: ESlint 0.18.0 compatibility...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.