Style error information

https://eslint.org/docs/rules/no-empty

Remark

Code refactoring and Improvement

It merged the conditions.

** It only affects single file, I assigned to Quickedit for further checking by maintainer and contributors.

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

- N/A

CommentFileSizeAuthor
#14 eslint-no-empty-2914718-14.patch2.13 KBdroplet
#9 eslint-no-empty.patch2.13 KBdroplet
#7 eslint-no-empty.patch2.26 KBdroplet
#5 eslint-no-empty.patch2.28 KBdroplet
eslint-no-empty.patch2.29 KBdroplet
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

droplet created an issue. See original summary.

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ as a volunteer commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +JavaScript clean-up

I'd reverse the condition instead of returning early but it works the same.

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ as a volunteer commented
Issue summary: View changes
alexpott’s picture

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

I'd reverse it too... also the patch no longer applies.

droplet’s picture

droplet CreditAttribution: droplet commented
Status: Needs work » Needs review
FileSize
eslint-no-empty.patch2.28 KB

Status: Needs review » Needs work

The last submitted patch, 5: eslint-no-empty.patch, failed testing. View results

droplet’s picture

droplet CreditAttribution: droplet commented
Status: Needs work » Needs review
FileSize
eslint-no-empty.patch2.26 KB
alexpott’s picture

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

@droplet can we swap the logic around too?

I think we want something like this...

        .forEach((field) => {
          // Ignore the current field and other fields with the same view mode.
          if (field !== currentField && field.get('fieldID') !== currentField.get('fieldID')) {
            otherViewModes.push(field.getViewMode());
          }
        });
droplet’s picture

droplet CreditAttribution: droplet commented
FileSize
eslint-no-empty.patch2.13 KB

why not :)

The last submitted patch, 7: eslint-no-empty.patch, failed testing. View results

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott as a volunteer and commented 
+++ b/core/modules/quickedit/js/models/FieldModel.es6.js
@@ -218,15 +218,8 @@
+          if (field !== currentField || field.get('fieldID') !== currentField.get('fieldID')) {

I might be wrong but I think this needs to be &&

droplet’s picture

droplet CreditAttribution: droplet commented

Oh Oh. Made a simple mistake.

What do you think this version to keep separate comments:

          // Ignore the current field.
          if (field !== currentField) {
            // Also ignore other fields with the same view mode.
            if (field.get('fieldID') !== currentField.get('fieldID')) {
              otherViewModes.push(field.getViewMode());
            }
          }
dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner at Chapter Three commented
Status: Needs review » Needs work

Let's set the correct status at the moment.
At least for me #12 feels less readable actually.

droplet’s picture

droplet CreditAttribution: droplet commented
Status: Needs work » Needs review
FileSize
eslint-no-empty-2914718-14.patch2.13 KB

I prefer the early return in #7 and think that's more common in JS :p

It need not compute the && logic (0.00000000001s faster). So while on debugging, you see either one meet the condition, you got it!

dawehner’s picture

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

Let's go with it!

droplet’s picture

droplet CreditAttribution: droplet commented

GuardClause:
http://wiki.c2.com/?GuardClause

Refactoring: Improving the Design of Existing Code

  • xjm committed 7655c87 on 8.5.x 
    Issue #2914718 by droplet, alexpott, nod_, dawehner: JS codestyle: no-...
xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Status: Reviewed & tested by the community » Fixed

It's JavaScript even I can read!

The fact that the inline comment more closely matches the changed code in #14 makes me agree that this way around makes more sense.

I confirmed that:

  • No errors for this rule remain after applying the patch (based on the results of yarn lint:core-js-stats).
  • Introducing a new empty condition raises a new error with the above command.
  • The transpilation (is that a word?) target is updated correctly.

Committed and pushed to 8.5.x. Thanks!

Status: Fixed » Closed (fixed)

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