Style error information

https://eslint.org/docs/rules/no-mixed-operators

Remark

Code refactoring, no NON-ES6 changes is correct!

In `history.es6.js`, I extracted to a new variable. It's easier to understand!

How to Review

## 1. Apply Patch
## 2. Review Code Changes
## 3. Confirm no Code Standard Errors
yarn & yarn lint:core-js-passing
## 4.1 If `NO` errors, mark the issue as `Reviewed & tested by the community` (Don't be shy, we're all friendly)
## 4.2 If `HAS` errors, fix it and upload a new patch (Just do it and you can!!!)

Background

- #2912962: Step 1 JS codestyle: [meta] Fix JS coding standards in core

- We adapted the airbnb coding standard (#2815077: Adopt airbnb javascript style guide v14.1 as new baseline javascript coding standards for Drupal 8 core and contrib), but we are not fully compliant to it yet.

More Information

- Using ES6 in Core
https://www.drupal.org/node/2815083

- To find JS code standard errors stats
yarn & yarn lint:core-js-stats

Valuable Follow-up

- Extract conditions to named variables. Sometimes it's easier to understand. @see: https://dzone.com/articles/code-smells-if-statements
(It's not required all time anyway :p)

CommentFileSizeAuthor
#6 eslint-no-mixed-operators.patch5.5 KBdroplet
eslint-no-mixed-operators.patch5.5 KBdroplet
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

droplet created an issue. See original summary.

droplet’s picture

droplet CreditAttribution: droplet commented
Issue summary: View changes
droplet’s picture

droplet CreditAttribution: droplet commented
Status: Reviewed & tested by the community » Needs review
dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner at Chapter Three commented
Status: Needs review » Reviewed & tested by the community

For my own interest and the sake of reviewing this patch: OPerator precendence in javascript: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operat...

+++ b/core/modules/color/color.es6.js
@@ -124,7 +124,7 @@
-            given[1] = 1 - (1 - given[1]) * d;
+            given[1] = 1 - ((1 - given[1]) * d);

@@ -138,7 +138,7 @@
-            given[2] = 1 - (1 - given[2]) * d;
+            given[2] = 1 - ((1 - given[2]) * d);

These ones are weird. In theory the additional () wouldn't be needed because multiplication would be preferred over addition / subtraction anyway, but well, these are the eslint rules :)

I'm was a bit confused that some of the changes don't end up in changes of the actual JS files, but I guess that's how it is ...

$ npm run build:js
$ git s
M  .eslintrc.passing.json
M  misc/tabledrag.es6.js
M  modules/block/js/block.admin.es6.js
M  modules/color/color.es6.js
M  modules/history/js/history.es6.js
M  modules/history/js/history.js
M  modules/quickedit/js/views/EntityToolbarView.es6.js

Status: Reviewed & tested by the community » Needs work

The last submitted patch, eslint-no-mixed-operators.patch, failed testing. View results

droplet’s picture

droplet CreditAttribution: droplet commented
Assigned: Unassigned » droplet
FileSize
eslint-no-mixed-operators.patch5.5 KB

@dawehner,

that's ESLint limits I bet. Actually, I like the no parentheses version for some changes. Should we drop inline comment to skip the error?

** reroll to remove .eslintrc.passing.json conflicts only.

droplet’s picture

droplet CreditAttribution: droplet commented
Assigned: droplet » Unassigned
dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner at Chapter Three commented
Status: Needs work » Reviewed & tested by the community

that's ESLint limits I bet. Actually, I like the no parentheses version for some changes. Should we drop inline comment to skip the error?

Yeah I'm not sure its worth writing a comment vs adding some parenthesis :) I have no idea how hard it would be to fix this in eslint, but maybe the ideal/better approach would be to fix it upstream and then improve the code later.

The patch, at least for me, is fine as it is in its current state.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott as a volunteer and commented

Crediting @dawehner because #4 is helpful when reviewing the patch.

alexpott’s picture

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

Committed e113f8f and pushed to 8.5.x. Thanks!

  • alexpott committed e113f8f on 8.5.x 
    Issue #2914723 by droplet, dawehner: JS codestyle: no-mixed-operators

Status: Fixed » Closed (fixed)

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