web4bio / webgen

WebGen Vertically Integrated Project
https://web4bio.github.io/webgen/main/html/
11 stars 26 forks source link

standard style + linting + error checking #332

Closed kaczmarj closed 3 years ago

kaczmarj commented 3 years ago

is there any interest in using eslint for this project to enforce a common style and to check for potential errors? if we enabled eslint in our IDEs, then it would check our code for errors and also suggest fixes. we can set options in an eslint config file. it can even validate jsdoc comments for us... and this is something we could also run on pull requests to make sure they are up to snuff. any thoughts?

kaczmarj commented 3 years ago

one problem with this idea is that it requires an installation of eslint. people can install it with nvm --> install node --> npm install -g eslint. it's extra setup, but is it too much to ask of developers? nvm is a node version manager. i like it because it doesn't clutter my system -- everything goes into one directory, and if i want to, i can nuke that directory. see https://github.com/nvm-sh/nvm for linux/macOS and https://github.com/coreybutler/nvm-windows for windows.

i made an example config that could work for us

{
    "extends": "eslint:recommended",
    "env": {
        "browser": true,
        "es6": true,
        "jquery": true
    },
    "parserOptions": {
        "ecmaVersion": 2015
    },
    "rules": {
        // Validate jsdoc comments.
        "valid-jsdoc": "error",
        // Can't use a variable before it is defined.
        "no-use-before-define": "error",
        // No unused vars.
        "no-unused-vars": [
            "error",
            {
                // Unused globals are ok (e.g., functions that are used elsewhere).
                "vars": "local"
            }
        ],
        // Async functions must include await.
        "require-await": "error",
        // Use === and !== instead of == and !=.
        "eqeqeq": [
            "error",
            "smart"
        ],
        // Use let or const instead of var.
        "no-var": "error",
        // Prefer using const instead of let when possible.
        "prefer-const": "error",
        // Prefer template strings instead of string concatenation.
        "prefer-template": "warn",
        // Use double quotes when possible.
        "quotes": [
            "error",
            "double"
        ],
        // Enforce a consistent use of semicolons.
        "semi": [
            "error",
            "always"
        ],
        // Indent with this many spaces.
        "indent": [
            "error",
            2
        ],
        // Max line length.
        "max-len": [
            "error",
            100
        ]
    }
}

a different option is to run eslint on the project once, fix all of the errors, and submit those fixes as a PR. i think this would go a long way towards tidying up this code base -- and catching/preventing potential errors.

enemeth19 commented 3 years ago

This is tempting, and I think it warrants further discussion with the whole group!

ZhenghaoZhu commented 3 years ago

Was just thinking of adding this. I'm more familiar with these tools (https://www.danilucaci.com/blog/how-to-lint-and-test-code-using-git-pre-commit-hooks) but the tools suggested could definitely work. Also, adding a couple of npm packages should be fine.

kaczmarj commented 3 years ago

is it reasonable to care whether code uses var, let, or const? == vs ===? do i need help???

kaczmarj commented 3 years ago

closing because in our group discussion on october 22 2021, we decided that this sort of change would impose too great of a burden.