👌 IMPROVE: Overhaul of #309

[xcesiv]

Complete overhaul of #309 utilizing all of the best features of eslint, stylelint, and prettier.

The new linting commands are as followed:

  "lint": "npm run lint:js && npm run lint:style && npm run lint:prettier",
  "lint-staged": "lint-staged",
  "lint-staged:js": "eslint --ext .js,.jsx,.ts,.tsx ",
  "lint-staged:style": "prettier --write \"**/*.{js,jsx,tsx,ts,css,md,json}\"",
  "lint:fix": "eslint --fix --cache --ext .js,.jsx,.ts,.tsx --format=pretty ./src && npm run lint:style",
  "lint:js": "eslint --cache --ext .js,.jsx,.ts,.tsx --format=pretty ./src",
  "lint:prettier": "prettier --check \"src/**/*\" --end-of-line auto",
  "lint:style": "stylelint --fix \"src/**/*.css\"",

Located here:

https://github.com/xcesiv/lobster/blob/2f83adda41b90c0c7e3e5366f4def79c1b9e623f/package.json#L81-L88

If you run lint:js you will see the current lists of warnings. and errors. This .eslint.js s modeled heavily off my personal preferences using typescript, which are very strict with errors. Eslint will attempt to fix certain aspects of the typescript code especially in src/js/core directory if lint:fix is ran & breaking changes will occur due to discrepancies. in how it was written vs the configure breaks.

It should be noted that we have downgraded typescript from typescript@4.4.0 to typescript@4.2.4. This is to compile with @typscript-eslint message that appears upon run with v4.4.0. This caused no issues with any other code or packages except with ts-node due to being listed in the dependencies rather than devDependencies. As such, a clean install of the node_modules is recommended. The following steps to get everything working:

rm -rf node_modules package-lock.json
npm install
# To run each command separately you could run
npm run lint:prettier # To see the putput of prettier --check
npm run lint:style # To see the output of stylelint 
npm run lint:js # to see the errors and warning
# or you can just run
npm run lint
# however this goes in the order of eslint -> stylelint -> prettier
# Once the errors from eslint have been resloved or changed you can run
npm run lint:fix
# To fix any lint errors

You'll also see the functionality for lint-staged has been added. This will enable us to check authors code and fix it with prettier before they can push anywhere.

There is are errors produced by selector-class-pattern of stylelint due to the lobster.css not following strict kebab-case. Details on selector-class-pattern can be found in their documentation. For now, I have disabled selector-class-path in the file header.

TODO:

• [x] Run eslint locally
• [x] Convert errors to warnings where need be
• [x] Fix errors that are produced.

[jamesjuett]

I ended up needing to up the version of prettier in package.json. Works with "2.5.1".

[xcesiv]

So this utilizes @typescript/eslint-parser which replaces certain functionalties and acts just thee same as eslint does but to help with some of the issues of eslint within typescript thus why it appears to be extensive.

I can go through and set each one to either warn or to the default by eslint that's not a problem.

Starting with the recommended rules actually breaks your code significantly. As the photo on Discord suggested.

I can change the settings to what eslint's are by default if you'd like.

[xcesiv]

Overall, I'd prefer using a simpler .eslintrc that depends primarily on extending the recommended configuration as in #309 . Some of the options in the proposed .eslintrc here are the same as would be inherited, and others that differ are sometimes a bit too strict or opinionated for my taste, at least (or sometimes opposite my taste, e.g. no-return-else 😄). I also generally would prefer it to start with the recommended rules that are generally quite uncontroversial and consider a move toward stricter linting in the future.

Also why do you want no-else-return turned off? What circumstance do you return within an else statement? If a return happens before an else statement then it returns otherwise wouldn't it be unnecessary and should return outside of the else. That is default. What I changed wasallowElseIf: false instead of the default. Examplle:

function foo() {
    if (error) {
        return 'It failed';
    } else if (loading) {
        return "It's still loading";
    }
}

should be:

function foo() {
    if (error) {
        return 'It failed';
    }

    if (loading) {
        return "It's still loading";
    }
}

No need for an else statement.

Maybe there is a more complex statement I am not forshadowing :)

[xcesiv]

Overall, I'd prefer using a simpler .eslintrc that depends primarily on extending the recommended configuration as in #309 . Some of the options in the proposed .eslintrc here are the same as would be inherited, and others that differ are sometimes a bit too strict or opinionated for my taste, at least (or sometimes opposite my taste, e.g. no-return-else 😄). I also generally would prefer it to start with the recommended rules that are generally quite uncontroversial and consider a move toward stricter linting in the future.

Here'd the link to @typescript-eslint: https://typescript-eslint.io/

The problem with default rules is they're meant for javascript, not typescript.

[xcesiv]

Overall, I'd prefer using a simpler .eslintrc that depends primarily on extending the recommended configuration as in #309 . Some of the options in the proposed .eslintrc here are the same as would be inherited, and others that differ are sometimes a bit too strict or opinionated for my taste, at least (or sometimes opposite my taste, e.g. no-return-else 😄). I also generally would prefer it to start with the recommended rules that are generally quite uncontroversial and consider a move toward stricter linting in the future.

Also why do you want no-else-return turned off? What circumstance do you return within an else statement? If a return happens before an else statement then it returns otherwise wouldn't it be unnecessary and should return outside of the else. That is default. What I changed wasallowElseIf: false instead of the default. Examplle:

function foo() {
    if (error) {
        return 'It failed';
    } else if (loading) {
        return "It's still loading";
    }
}

should be:

function foo() {
    if (error) {
        return 'It failed';
    }

    if (loading) {
        return "It's still loading";
    }
}

No need for an else statement.

Maybe there is a more complex statement I am not forshadowing :)

Here is an example from statements.ts

  public static createFromAST(ast: IfStatementASTNode, context: BlockContext): IfStatement {
    let condition = createExpressionFromAST(ast.condition, context);

    // If either of the substatements are not a block, they get their own implicit block context.
    // (If the substatement is a block, it creates its own block context, so we don't do that here.)
    let then =
      ast.then.construct_type === 'block'
        ? createStatementFromAST(ast.then, context)
        : createStatementFromAST(
            {
              construct_type: 'block',
              source: ast.then.source,
              statements: [ast.then],
            },
            context
          );

    if (!ast.otherwise) {
      // no else branch
      return new IfStatement(context, ast, condition, then);
    } else {
      // else branch is present
      // See note above about substatement implicit block context
      let otherwise =
        ast.otherwise.construct_type === 'block'
          ? createStatementFromAST(ast.otherwise, context)
          : createStatementFromAST(
              {
                construct_type: 'block',
                source: ast.then.source,
                statements: [ast.otherwise],
              },
              context
            );

      return new IfStatement(context, ast, condition, then, otherwise);
    }
  }

Dropping the else statement returns:

  public static createFromAST(ast: IfStatementASTNode, context: BlockContext): IfStatement {
    let condition = createExpressionFromAST(ast.condition, context);

    // If either of the substatements are not a block, they get their own implicit block context.
    // (If the substatement is a block, it creates its own block context, so we don't do that here.)
    let then =
      ast.then.construct_type === 'block'
        ? createStatementFromAST(ast.then, context)
        : createStatementFromAST(
            {
              construct_type: 'block',
              source: ast.then.source,
              statements: [ast.then],
            },
            context
          );

    if (!ast.otherwise) {
      // no else branch
      return new IfStatement(context, ast, condition, then);
    }
    // else branch is present
    // See note above about substatement implicit block context
    let otherwise =
      ast.otherwise.construct_type === 'block'
        ? createStatementFromAST(ast.otherwise, context)
        : createStatementFromAST(
            {
              construct_type: 'block',
              source: ast.then.source,
              statements: [ast.otherwise],
            },
            context
          );

    return new IfStatement(context, ast, condition, then, otherwise);
  }

Both of which compile the same way.

[jamesjuett]

Starting with the recommended rules actually breaks your code significantly. As the photo on Discord suggested.

Huh, that's odd. On #309 everything builds fine for me after running the formatter and linter, e.g.

npm run fix-code
npm run build

Granted, there's some differences between the tooling I had set up there and yours, but I don't understand in principle why using the recommended rules would just break the code. I wonder if something else was going on?

[jamesjuett]

Here'd the link to @typescript-eslint: https://typescript-eslint.io/

The problem with default rules is they're meant for javascript, not typescript.

Ope. I wasn't very clear there. By default I mean relying on something like this (from #309):

{
  "root": true,
  "extends": [
      "eslint:recommended",
      "plugin:@typescript-eslint/recommended"
  ],
  "parser": "@typescript-eslint/parser",
  "parserOptions": { "project": ["./tsconfig.json"] },
  "plugins": [
      "@typescript-eslint"
  ]
}

And each additional rule added to change something from the recommended ones from eslint and typescript-eslint would need to be justified.

[jamesjuett]

Also why do you want no-else-return turned off? What circumstance do you return within an else statement? If a return happens before an else statement then it returns otherwise wouldn't it be unnecessary and should return outside of the else. That is default. What I changed wasallowElseIf: false instead of the default.

Mostly because I think functions like this are aesthetically pleasing :)

  public parse(s: string): ParsingResult<this> {
    if (s.length > 0) {
      return createSuccessParsingResult(new Value(s.charCodeAt(0), this, true));
    } else {
      return createErrorParsingResult();
    }
  }

[xcesiv]

I have fixed the issues that break the code. The recommended settings were in my .eslintrc.js file, just scattered throughout and changed in some ways. You will see in the new version, I have setup the extension of the recommended settings per your request, then I added all of the recommended rules tot he top of the rules section. First for eslint, then for typescript-eslint. I commented out which ones that typescript-eslint replace of eslint first, then a section where I modified then, then a final one that is additions I have made from each.

The reason it was breaking it was because prettier couldn't handle some of your comment that were inline in some cases properly. Particularly in 14f008c, I address the issue by moving the comment around.

Also, I'm not quite sure but it says we have merge conflicts with each other. My .eslintrc.js file is your eslintrc.json file renamed, and I'm not sure not to resolve the conflict. I did try in the last commit without success. I image this is because you've updated it since I changed the name in my PR? If you know how to address this let me know.

I've changed the commands to be more fluent as well:

        "lint": "npm run lint:js && npm run lint:style && npm run lint:prettier",
        "lint:fix": "eslint --fix --cache --ext .js,.jsx,.ts,.tsx --format=pretty ./src && stylelint --fix \"src/**/*.css\" && prettier --write \"**/*.{js,jsx,tsx,ts,css,md,json}\"",
        "lint:js": "eslint --cache --ext .js,.jsx,.ts,.tsx --format=pretty ./src",
        "lint:prettier": "prettier --check \"src/**/*\" --end-of-line auto",
        "lint:style": "stylelint \"src/**/*.css\"",
        "lint-staged": "lint-staged",
        "lint-staged:js": "eslint --fix --cache --ext .js,.jsx,.ts,.tsx",
        "lint-staged:prettier": "prettier --write \"**/*.{js,jsx,tsx,ts,css,md,json}\"",
        "lint-staged:style": "stylelint --fix \"src/**/*.css\"",

If we run npm run lint it will not attempt to fix anything, but will lint through all the typescript looking for errors/warnings with prettier syntax and report it, then the css, then check if the code meets prettier standards. lint:fix will fix attempt to fix all said issues.

lint-staged will run if we include precommit handling, but I did not add that here yet, but I can. It would just be adding the pre-commit package to the repo and including:

  "pre-commit": [
    "lint-staged"
  ],

to package.json

[xcesiv]

Also, I'm not quite sure but it says we have merge conflicts with each other. My .eslintrc.js file is your eslintrc.json file renamed, and I'm not sure not to resolve the conflict. I did try in the last commit without success. I image this is because you've updated it since I changed the name in my PR? If you know how to address this let me know.

I figured it out.