Style error information

https://eslint.org/docs/rules/no-useless-escape

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

CommentFileSizeAuthor
#9 drupal_—_-bash_—_80×38.png460.88 KBxjm
#7 2916294-7.patch8.7 KBdawehner
#4 2916294-3.patch8.74 KBdawehner
#2 2916294-2.patch5.1 KBdawehner

Comments

dawehner created an issue. See original summary.

dawehner’s picture

dawehner
Primary language German
commented
StatusFileSize
new 2916294-2.patch5.1 KB

Here is some patch

GrandmaGlassesRopeMan’s picture

GrandmaGlassesRopeMan
he/him
Primary language English
Location 🌎
commented

@dawehner Is the transpiled JS missing from this patch?

dawehner’s picture

dawehner
Primary language German
commented
StatusFileSize
new 2916294-3.patch8.74 KB

oops, I forgot the transpiling.

GrandmaGlassesRopeMan’s picture

GrandmaGlassesRopeMan
he/him
Primary language English
Location 🌎
commented
Status: Needs review » Reviewed & tested by the community

I think this looks alright. Some of these were holdovers before template strings. 👍

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 4: 2916294-3.patch, failed testing. View results

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs work » Reviewed & tested by the community
StatusFileSize
new 2916294-7.patch8.7 KB

Falling back to patching base and 3-way merge...🎆

Here is a quick reroll.

  • xjm committed 7fbb5e9 on 8.5.x 
    Issue #2916294 by dawehner: JS codestyle: no-useless-escape
xjm’s picture

xjm
she/her
Primary language English
commented
Issue summary: View changes
Status: Reviewed & tested by the community » Fixed
StatusFileSize
new drupal_—_-bash_—_80×38.png460.88 KB

This issue has been an exciting test of my regex skills, both in terms of when escaping is and isn't needed for special characters, and in crafting a word diff to diff them. (As well as a test of my eyesight.)

Turns out the word diff is actually super simple. Let's call it a "character diff", even:
git diff --staged --color-words="."

The only changes are removed \ in places that https://eslint.org/docs/rules/no-useless-escape says they can be removed safely. The aforementioned page helped reassure me that it is in fact safe to not use \ on [ inside a character class. I also learned something. I did not expect the following to work, but the page reassures me that it will not result in a behavior change:

In PHP it would have been, just, like, wrong (in HEAD that is).

I read over the changes carefully and linted before and after. Before:

Errors:
==============================
no-use-before-define: 75
max-len: 70
camelcase: 65
no-restricted-syntax: 54
no-shadow: 42
no-useless-escape: 19
new-cap: 18
default-case: 16
no-continue: 9
no-new: 9
no-throw-literal: 1

After:

Errors:
==============================
no-use-before-define: 75
max-len: 70
camelcase: 65
no-restricted-syntax: 54
no-shadow: 42
new-cap: 18
default-case: 16
no-continue: 9
no-new: 9
no-throw-literal: 1

So, committed and pushed to 8.5.x. Thanks!

dawehner’s picture

dawehner
Primary language German
commented

Turns out the word diff is actually super simple. Let's call it a "character diff", even:
git diff --staged --color-words="."

That's super cool. Thank you for sharing this! Now I jut need to remember it by thinking about no useless escapes.

I did not expect the following to work, but the page reassures me that it will not result in a behavior change:

Me neither to be honest, I'm glad eslint helps us to catch these differences between languages.

Status: Fixed » Closed (fixed)

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