audiojs / contributing

Discussion and guidelines for contributing
2 stars 0 forks source link

ESLint #19

Closed dy closed 8 years ago

dy commented 8 years ago

We have to decide and set up code style, mostly to avoid greenkeeper/travis fails.

Things to check:

Basically anything which may mislead reader.

ahdinosaur commented 8 years ago

obligatory standard.

but honestly i've learned code style is best left up to those who wrote the code, which is not me. :smile:

dy commented 8 years ago

I would opt for semistandard with tabs, otherwise I feel pain navigating through code and constant dealing with extra/lacking space here and there in indents, but I would not force it even within a single package to enable comfortable collaboration. Whereas stricts etc wreck greenkeeper, which inflicts pain on all of us in the case

dy commented 8 years ago

Things which I would not check:

Basically, anything which may cause unnecessary pain or prevent easy running npm test during development.

jamen commented 8 years ago

Agreed. We should have it trigger for bad/confusing practices instead of for stylish ones, might be the approach to take.

dy commented 8 years ago

Here is .eslintrc.json with some rules set to warning instead of error (they can arguably break usual workflow, like running tests etc):

{
    "env": {
        "browser": true,
        "node": true,
        "commonjs": true,
        "es6": true
    },
    "extends": "eslint:recommended",
    "rules": {
        "strict": 2,
        "indent": 0,
        "linebreak-style": 0,
        "quotes": 0,
        "semi": 0,
        "no-console": 1,
        "no-cond-assign": 1,
        "no-constant-condition": 1,
        "no-duplicate-case": 1,
        "no-empty": 1,
        "no-ex-assign": 1,
        "no-extra-boolean-cast": 1,
        "no-extra-semi": 1,
        "no-fallthrough": 1,
        "no-func-assign": 1,
        "no-global-assign": 1,
        "no-implicit-globals": 2,
        "no-inner-declarations": ["error", "functions"],
        "no-irregular-whitespace": 2,
        "no-loop-func": 1,
        "no-multi-str": 1,
        "no-mixed-spaces-and-tabs": 1,
        "no-proto": 1,
        "no-sequences": 1,
        "no-throw-literal": 1,
        "no-unmodified-loop-condition": 1,
        "no-useless-call": 1,
        "no-void": 1,
        "no-with": 2,
        "wrap-iife": 1,
        "no-redeclare": 1,
        "no-unused-vars": ["error", { "vars": "all", "args": "none" }],
        "no-sparse-arrays": 1
    }
}

Not 100% sure about them though, maybe we should force some and use //eslint-disable instead. Which other rules should we add?

Also package.json scripts:

    "preversion": "npm run lint && npm test",
    "lint": "eslint *.js --ignore-pattern test*",
dy commented 8 years ago

If we're agreed on that .eslintrc.json, we can close this issue.

jamen commented 8 years ago

Yup, agree on those rules. Nice work. :)