New linter

[HappyZombies]

As discussed per #5 ,we are looking into implementing ESLint as our linter.

In the original project, it appeared that ESLint was going to be part of some version.

https://github.com/oauthjs/node-oauth2-server/blob/dev/.eslintrc

However this never got merged into master.

The goal here is to

1. Define a eslint config we all can agree on
2. Then create a PR that contains the ESLint config along with all additional fixes / formatting we implement (if possible).

[HappyZombies]

Let's keep in mind that the linter might actually throw too many errors on the application, and will cause our GitHub actions to fail if they are part of the pipeline.

In such a case, we might just need to implement the ESLint config file and look into addressing the issues the linter brings up at a later time.

[jwerre]

Here's the lint file I propose. I'll just post this to start the discussion.

{
    "extends": "eslint:recommended",
    "env": {
        "node": true,
        "mocha": true,
        "es6": true
    },
    "parserOptions": {
        "ecmaVersion": 9,
        "sourceType": "module",
        "ecmaFeatures" : {
            "globalReturn": false,
            "impliedStrict": true,
            "jsx": false
        }
    },
    "rules": {
        "indent": [
            "error",
            "tab" // I like tabs but I get it if you don't
        ],
        "linebreak-style": [
            "error",
            "unix"
        ],
        "quotes": [
            "error",
            "single"
        ],
        "semi": [
            "error",
            "always"
        ],
        "no-console": "off",
        
        "no-unused-vars": [
            "error", 
            { 
                "vars": "all",
                "args": "none",
                "ignoreRestSiblings": false 
            }
        ]
    }
}

[jankapunkt]

I have some additions, for example I'd prefer spaces instead of tabs (because they can also be wrapped by ides with tabsize) and I'd also turn on no-console for all non-warning and non-error console usage. I also would like to propose stoustroup style for if else because it makes commenting blocks very clear to understand. I can provide example and a PR if you like

[jwerre]

@jankapunkt what is the "stoustroup style". Example please.

[HappyZombies]

+1 for spaces
+1 for turning on console

For Stroustrup I am willing to give it a shot, personally never used but this doesn't bother me that much.

@jwerre https://eslint.org/docs/rules/brace-style example of Stroustrup.

Can we also talk about 2 spaces v 4 spaces? Personally for me 2, but I am ok with either.

[oklas]

Stroustrup is author of the C++, he suggests to write:

    if (x < 0) {
    }
    else {
    }

instead of

    if (x < 0) {
    } else {
    }

[jankapunkt]

// comment on if block here
if (condition) {
  // ...
}

// Comments on else block
else {
  // ...
}

[oklas]

Spaces for indent and size 2 are widely used today and are very clean and convenient.
I've never seen Stroustrup's approach in js projects or any other not so often used.

[jwerre]

Are we taking 2 spaces or 4?

[jwerre]

{
  "extends": "eslint:recommended",
  "env": {
    "node": true,
    "mocha": true,
    "es6": true
  },
  "parserOptions": {
    "ecmaVersion": 9,
    "sourceType": "module",
    "ecmaFeatures" : {
      "globalReturn": false,
      "impliedStrict": true,
      "jsx": false
     }
  },
  "rules": {
    "indent": [
      "error",
      2
    ],
    "brace-style": [
      "error",
      "stroustrup"
    ],
    "linebreak-style": [
      "error",
      "unix"
    ],
    "quotes": [
      "error",
      "single"
    ],
    "semi": [
      "error",
      "always"
    ],
    "no-console": "off",

    "no-unused-vars": [
      "error", 
      { 
        "vars": "all",
        "args": "none",
        "ignoreRestSiblings": false 
      }
    ]
  }
}

[oklas]

2 would be good, seems project already uses 2

[jankapunkt]

as @oklas wrote 2 spaces are a good practice. Regarding stoustroup I think it's also rather a personal flavour, since I like commenting on each branching block but let's stick to what many projects do so many people are comfortable with it = higher chances of contributions

[jwerre]

With "stroustrup" there are 412 problems to fix, without only 47.

... just saying.

[HappyZombies]

Can't most those be covered with the auto fix? Well I guess we still gotta be careful

[jwerre]

Why don't we get eslint implemented and open up a new PR for stroustrup?

[HappyZombies]

Yup, definitely. That's why I mentioned we have a PR for eslint, then another one doing the autofixes/fixing errors on eslint.

[oklas]

Every change in the code, even if it is spaces, and even more so even a simple change in parentheses adds a huge number of differences and complicates the comparison with the original source code of fork.

[jwerre]

This Regex pops up as a lint error but I'm reluctant to make any changes to it. I'm going to add "no-control-regex": "off", to the config if it's ok with you guys.

[jankapunkt]

Yes please let's keep stoustroup out of the game for now and focus on whats easily manageable.

[jankapunkt]

@jwerre let's rather add an eslint exception to this line? The eslint docs say this is likely a mistake of inapproproate usage, which I highly doubt for this specific line but I think the rule is good in general and should apply, especially for newcomers that could do this mistake