📝 Enumerating eslint rules + package refactoring #1

camilleriluke · 2016-12-22T08:51:52Z

This is a non breaking change.

Followed the basic structure of eslint-config-walmart and migrated our rules.

What changed

Split the rules into categories according to http://eslint.org/docs/rules/
Added supports for different configurations (es5, es5-test, es6, etc.)
Added support for commitizen
Added validation on commit messages (husky and validate-commit-msg)
Added eslint as a dependency and peer dependency
Added npm scripts
- npm run commit for those who don't have commitizen installed globally
- npm run lint self linting
- npm run find-new-eslint-rules - checks for new (missing) rules inside for all the configurations
- npm run test - lints and checks for rules
Pimped up README
Added a license files
Added ignore on debug files

More things to look into

Add contribution guide
- How to commit
Split envs into separate files
Add support for eslint-find-rules
~~Add travis support (maybe this shouldn't be part of this PR)~~ Moved to issue https://github.com/Casumo/eslint-config-casumo/issues/2
~~Add version bumping (maybe this shouldn't be part of this PR)~~ Moved to issue https://github.com/Casumo/eslint-config-casumo/issues/3

Ignoring debug files

Added support for commitizen, and validation for commit messages. Also added eslint as a dev dependency and peer dependency

Updated readme to be more expressive

Added all rules according to http://eslint.org/docs/rules/

leventebalogh

Our first PR 🚀 🥇
Nice job! :)

leventebalogh · 2016-12-22T09:57:29Z

README.md

+[![version](https://img.shields.io/npm/v/eslint-config-casumo.svg?style=flat-square)](https://www.npmjs.com/package/eslint-config-casumo)
+[![downloads](https://img.shields.io/npm/dm/eslint-config-casumo.svg?style=flat-square)](http://npm-stat.com/charts.html?package=eslint-config-casumo&from=2015-08-01)
+[![MIT License](https://img.shields.io/npm/l/eslint-config-casumo.svg?style=flat-square)](http://opensource.org/licenses/MIT)
+[![Commitizen friendly](https://img.shields.io/badge/commitizen-friendly-brightgreen.svg)](http://commitizen.github.io/cz-cli/)


Nice, just wanted to recommend the commitizen-friendly badge, but see it's already there 😛

leventebalogh · 2016-12-22T10:00:13Z

best-practices.js

@@ -0,0 +1,77 @@
+module.exports = {


I think it would be nice to move these rule categories to a sub-directory, like /rules

leventebalogh · 2016-12-22T10:01:44Z

best-practices.js

+        browser: true,
+        node: true
+    },
+    rules: {


I think we shouldn't include any environment setup in the rules themselves to make them more composable.
So like:

module.exports = { rules: { /* ... */ } }

leventebalogh · 2016-12-22T10:05:09Z

best-practices.js

+        // NO_RULE 'require-await': 2,
+        'vars-on-top': 2,
+        'wrap-iife': 2
+        // 'yoda': 2


This condition name yoda still makes me laugh 😄

leventebalogh · 2016-12-22T10:08:01Z

best-practices.js

+        // 'accessor-pairs': 2,
+        // 'array-callback-return': 2,
+        // 'block-scoped-var': 2,
+        // NO_RULE 'class-methods-use-this': 2,


I don't really agree that we should add all the rules we don't use commented out. This makes it harder to read, and requires you to read the README to understand why are they commented out and to get what NO_RULE means. Also we have to maintain the commented out rules as well as ESLint is updating.

I would rather just remove these rules for now (the ones we don't use) and add them back 1 by 1 by PRs when we feel the need for them. Then the conversation on that specific rule can be tracked down in the PR itself.

I the main reason I added this is for ease of use.

Pre this PR someone would have to cross reference the rules with the eslint site and copy and paste the config files in here (check if already exists and so on). My intention was to make it easy for someone to look at the category file (f.ex stylistic.js) and get a fast clear picture of what are the possible rules they can enabled.

I will add the suggested workflow to follow in a contribution guide. That would give all the information to anyone who would like to contribute.

leventebalogh · 2016-12-22T10:12:43Z

index.js

+        './stylistic-issues.js',
+        './ecma-script-6.js'
+    ],
+    rules: {},
    globals: {


I think globals setup should go with environments, e.g. we have different globals needed for a test environment, for a browser and a node environment.

leventebalogh · 2016-12-22T10:21:35Z

I don't know the right solution yet, I am just starting out from that I would like to use something like this:

// tests/.eslintrc
{
    "extends": [
        "casumo",
        "casumo/es6"
        "casumo/mocha"
    ]
}

// gulp/.eslintrc
{
    "extends": [
        "casumo",
        "casumo/es6"
        "casumo/node"
    ]
}

So the environment specific stuff would be only in casumo/node and casumo/mocha in this case. What do you think about that?

camilleriluke · 2016-12-23T12:04:01Z

Had a change of heart and completely changed the direction.

Now we have

./configurations all configurations every application should ideally pick from here
./rules rules grouped by plugin type
./rules/eslint eslint rules split by categories
./rules/mocha relevant mocha rules

So you could use it like this:

By default it is es5 config

{
    "extends": "casumo"
}

or

{
    "extends": "casumo/configurations/es5-browser"
}

and in the gulp folder

{
    "extends": "casumo/configurations/es5-node"
}

Also all rules are enabled (no commented rules), this is to prepare for the check for missing rules npm script

…ders

leventebalogh · 2016-12-23T13:20:14Z

configurations/es5-test.js

@@ -0,0 +1,13 @@
+module.exports = {
+    extends: [
+        '../configurations/es5.js',


./es5.js would be enough?

haha I was following the pattern ../ to be consistent and easy on the eye 😄

this looks ugly 😛

extends: [ './es5.js', '../rules/mocha/on.js' ],

Ahh, ok, I thought it was accidental.
Yea, it's really easier to read that way and communicates the context. Agreed :)

leventebalogh · 2016-12-23T13:20:20Z

configurations/es5-node.js

@@ -0,0 +1,12 @@
+module.exports = {
+    extends: [
+        '../configurations/es5.js',


./es5.js would be enough?

same as #1 (comment)

leventebalogh · 2016-12-23T13:20:56Z

configurations/es5.js

+        '../rules/eslint/strict-mode/on.js',
+        '../rules/eslint/stylistic-issues/on.js',
+        '../rules/eslint/variables/on.js',
+        '../rules/eslint/deprecated/off.js'


Why do we require in turned off rules here?
'../rules/eslint/deprecated/off.js'

To get a green light from eslint-find-rules.

Currently it is throwing an error if the deprecated rules are not included.

Waiting on sarbbottam/eslint-find-rules#172 to be resolved and then we can safely remove them

Ahh, ok, I see now. Can we add a comment or a todo to these, so they are describing why they are there?

leventebalogh · 2016-12-23T13:21:25Z

configurations/es6-node-test.js

@@ -0,0 +1,18 @@
+module.exports = {
+    extends: [
+        '../configurations/es6-test.js'


'./es6-test.js'?

same as #1 (comment)

leventebalogh · 2016-12-23T13:22:11Z

configurations/es6-node-test.js

+    },
+    globals: {
+        expect: true,
+        sandbox: true


Shouldn't we add

describe: true, it: true

as well?

env mocha does the trick :)

aaa, cool I missed that :P 👍

leventebalogh · 2016-12-23T13:28:23Z

configurations/es5.js

+        '../rules/eslint/best-practices/on.js',
+        '../rules/eslint/possible-errors/on.js',
+        '../rules/eslint/ecma-script-6/off.js',
+        '../rules/eslint/node-js-and-common-js/off.js',


Is there any benefit of requiring in the turned off rules explicitly here?

Yes because the idea is to always ship a full set of rules including on and off.

Like this we are explicitly ignoring ecma-script-6 and node-js-and-common-js rules.

leventebalogh · 2016-12-23T13:28:52Z

configurations/es6-node.js

@@ -0,0 +1,24 @@
+module.exports = {
+    extends: [
+        '../configurations/es6.js',


Relative path ./es6.js?

same as #1 (comment)

leventebalogh · 2016-12-23T13:29:35Z

configurations/off.js

@@ -0,0 +1,20 @@
+module.exports = {


Why do we need a composition file for the turned off rules?

This would be useful if someone wants to have only small number of rules turned on. Composing a config file with off.js and their overrides

Hmm... I think it is descriptive enough if you only import / define the rules that you would like to enable. As no eslint rules are enabled by default, when would someone need to import this?

As no eslint rules are enabled by default, when would someone need to import this?

If you do not import all the off rules then eslint-find-rules would fail with the -u option

leventebalogh · 2016-12-23T13:35:54Z

One thing that I miss is the contribution guideline for it, as I don't really get the main differences between on and off rules, and how to update / use them, so I think new people wouldn't get it as well.

First I thought that the off rule files are containing rules that we don't use, however I saw that the on files contain also the same rules. Are they used to turn off categories explicitly?

Updated contribution steps and added better explenation of the configuration structure

Version bump to 1.1.0

camilleriluke · 2017-01-09T15:32:31Z

PR changes complete. Bumped the version to v1.1.0.

Waiting for 👍 so I will merge it in and publish the package.

Luke Camilleri added 9 commits December 22, 2016 01:36

docs(license): Add license file

6dadda9

refactor(.gitignore): Add more ignores

3b8aebd

Ignoring debug files

refactor(package.json): Update package.json tag values

47be9e6

refactor(package.json): Add new dev dependencies

ae27671

Added support for commitizen, and validation for commit messages. Also added eslint as a dev dependency and peer dependency

docs(README): Update readme

8ca9190

Updated readme to be more expressive

refactor(rules): Enumerated all eslint rules

2690ec8

Added all rules according to http://eslint.org/docs/rules/

docs(README): Update docs

ceb1ea9

feat(rules): Add es6 eslint rules

3ec2407

refactor(rules): Bundle es6 extenders for easy importing

89851ab

leventebalogh suggested changes Dec 22, 2016

View reviewed changes

Luke Camilleri added 4 commits December 23, 2016 09:27

refactor(rules): Move rules from root to rules folder

4de8271

chore(.edtiroconfig): Add .editorconfig

a0ef622

refactor(rules): Split rules into configuration for better composability

f28d851

refactor(rules): Match rule folder categories to eslint documentation

8226d7e

Luke Camilleri added 3 commits December 23, 2016 13:21

refactor(rules): Move deprecated rules inside the configuration exten…

a95dd2a

…ders

build(npm-scripts): Add test npm script

649efcf

chore(.npmignore): Add .npmignore

fb65164

camilleriluke force-pushed the enumerating-linting-rules branch from 44f9784 to fb65164 Compare December 23, 2016 12:47

leventebalogh reviewed Dec 23, 2016

View reviewed changes

leventebalogh approved these changes Dec 23, 2016

View reviewed changes

camilleriluke force-pushed the enumerating-linting-rules branch 3 times, most recently from c80e1cc to eb439b5 Compare December 23, 2016 15:32

Luke Camilleri added 3 commits December 24, 2016 18:01

docs(README): Update docs

37afb4a

refactor(rules): Add comment on why deprecated/off is needed

a9bebb4

refactor(rules): Update relative paths to configurations folder

72a3ab1

camilleriluke force-pushed the enumerating-linting-rules branch from eb439b5 to 72a3ab1 Compare December 24, 2016 17:01

Luke Camilleri added 2 commits January 9, 2017 14:48

docs(README): Improve documentation

5e7d41b

Updated contribution steps and added better explenation of the configuration structure

chore(package.json): Bump version

2cb604d

Version bump to 1.1.0

camilleriluke changed the title ~~📝 Enumerating eslint rules~~ 📝 Enumerating eslint rules + package refactoring Jan 9, 2017

camilleriluke merged commit b3951fd into casumo-archive:master Jan 9, 2017

camilleriluke deleted the enumerating-linting-rules branch January 9, 2017 16:08

📝 Enumerating eslint rules + package refactoring #1

📝 Enumerating eslint rules + package refactoring #1

Conversation

camilleriluke commented Dec 22, 2016 • edited Loading

More things to look into

leventebalogh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leventebalogh commented Dec 22, 2016

camilleriluke commented Dec 23, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

camilleriluke Dec 23, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leventebalogh commented Dec 23, 2016

camilleriluke commented Jan 9, 2017

camilleriluke commented Dec 22, 2016 •

edited

Loading

camilleriluke Dec 23, 2016 •

edited

Loading