Step 0 JS codestyle: Exclude non passing eslint rules

This is a very bad idea. The testbot could bypass it with a custom config.

It just introduces more errors on new JS patches since the local editor lost error hints

Don't want to hijack your guys' work and slow down the progress. But if we only used these commands temporarily. Adding a -prefix would make less confusing to other contributors. They won't adopt it to their contrib projects.

And this is make more sense to fix Errors first (and may be only)

Some rules, for example, the "no-mutable-exports". IMO, this is not useful for the CORE but good for Contrib. It seems not right to remove it from CORE .eslintrc forever. We might have to discuss in somewhere.

Errors:
==============================
no-use-before-define: 75
max-len: 70
camelcase: 66
no-restricted-syntax: 54
no-shadow: 42
newline-per-chained-call: 35
no-var: 25
vars-on-top: 24
no-mixed-operators: 24
no-useless-escape: 19
new-cap: 18
default-case: 16
no-multi-assign: 14
no-continue: 9
no-new: 9
prefer-const: 6
prefer-rest-params: 3
padded-blocks: 2
array-callback-return: 2
no-lonely-if: 2
comma-dangle: 2
no-confusing-arrow: 2
no-empty: 1
no-throw-literal: 1
no-useless-concat: 1





Warnings:
==============================
func-names: 467
no-mutable-exports: 145
no-unused-vars: 121
no-plusplus: 16

Attached a stat script. IMO, it's good to commit it with this issue together (, rather than chop into a new issue).

Note 1: I have a codemod for camelcase, let me know if you need my help. Cheers!
Note 2: There are few rules are formatting errors, eg. max-len, newline-per-chained-call, padded-blocks. IMO, we should group it in one slot. It's hard to introduce new bugs!

Isn't that what the patch is doing right now?

I mean a prefix like: "temp:lint:core-js-whatever'

Have a look at #2912962: [pp-1] Step 1 JS codestyle: [meta] Fix JS coding standards in core how we can make the life easy for committers. We will have one issue per rule, so I guess you could open up the camelcase rule

I will wait for your guy do the first and then I copy it. BTW, I think camelcase will be the last to resolve. Renaming the var/prop is API changes in many cases. It will break contrib. (or D8.5 only)

I'm glad the numbers you produced are the same as the one I put into the issue summary for myself. Yeah!

👍👍👍

At least from my point of view we should fix one rule per issue, as otherwise, reviews would be harder. Please let's optimize for reviews, as this is the rare resource.

If committers stick with the policy, 3 days per commit, it would be ultra slow and that means each time invalidate our other patches again. The re-roll game is not fun :)

The reason I trying to group them together is to lower the conflicts between patches and easier for commit.

The history told me it's not good. We failed at JS native selectors clean up, some JS winter cleanup/ rewrite, and the recent CSS clean up. (Strictly, I think the Theme CSS standard (to BEM) has failure also)

https://www.drupal.org/node/2865971#comment-12098104 (4 months, zero new commits)

#11

Attached a stat script. IMO, it's good to commit it with this issue together (, rather than chop into a new issue).

Awesome. I've included it with just a bit of code-style cleanup.

---

@dawehner I modified your latest patch slightly. I don't think we should be excluding warnings. They shouldn't cause the test bot to fail and most of them aren't critical. I also added indent to the ignore list; it appeared to be missing initially.

Yes.

I've removed the need for Needs subsystem maintainer review. However this should probably result in a change record since we are temporarily changing our existing rules for linting JavaScript.

Thanks @drpal. Yeah, that stat script can improve a bit still. But as a temp script, I will leave it as it :)

However this should probably result in a change record since we are temporarily changing our existing rules for linting JavaScript.

We only change the testbot, I guess? No? It's loose version of our CORE standard. It's okay to without change record I think.

Make it easier to review

Manual Interdiff:
- Removed indent
- Add --quiet to mute warning report

@droplet How come we don't want to track indent issues?

I removed indent and run, no errors. (D8.5)

@droplet I applied the patch from #20 against 12a28a5eba and there are still 21 errors with indentation.

Ahh. I think you running the new version: #2910705: Update JS Build Script Packages (I notice that before)

Okay. We could add it back for future.

** And there some new autofix in the new ESLINT

@droplet

Yep. I was running a 4.x.x branch of eslint. I'm fine with this as it is right now and if we ever get around to updating the dependencies we can just adjust these rules then.

Once this issue gets committed, I will publish 9 new issues. These issues I picked has less controversial (and try to avoid patch conflicts), we can commit them straightforward usually. 🚀🚀🚀🚀🚀

👏

This seems like a good way to help the process of fixing the coding standard violations.

Committed d0fa5b7, pushed to 8.5.x and published the change record. Thanks!

publish the change record