Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce ESLint #10

Merged
merged 8 commits into from Jan 25, 2017
Merged

Introduce ESLint #10

merged 8 commits into from Jan 25, 2017

Conversation

backflip
Copy link
Collaborator

@backflip backflip commented Nov 1, 2016

  • Add ESLint, remove JSHint and JSCS. Reasons:
  • Generate an .eslintrc from previous .jshintrc and .jscs files (using https://www.npmjs.com/package/polyjuice). Minor adaptations where needed (see specific commit).
  • Adapt code base to new ESLint config (where the config could not be adapted):
    • Having one single var declaration but separating require is not (yet) possible with ESLint (might be fixed in the future Support separate requires in one-var eslint/eslint#6175). That's why I have combined these declarations in the .data.js files. An alternative would be to disable the one-var rule.
    • Some very specific details did start to throw errors. I have "solved" this using // eslint-disable-line

@orioltf
Copy link
Member

orioltf commented Dec 15, 2016

I did use "extends": "eslint:recommended" (http://eslint.org/docs/rules/) for another project. How would you see that? It sets some rules that we can then overwrite with ours. This produces quite a shorter .eslintrc file.

I would also suggest to sort the rules alphabetically. This is as well how they are presented in their documentation and adds some easiness and expectation when reading the configuration.

"Promise": false
},
"rules": {
"no-bitwise": 2,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to the documentation http://eslint.org/docs/rules/no-bitwise and even to previous versions of the documentation (https://github.com/eslint/eslint/blob/master/docs/rules/no-bitwise.md) "no-bitwise" should not admit a number as a value. Additionally, configuring "extends": "eslint:recommended" would also bring this by default as an error, and if ESLint changes the API for this rule we wouldn't need to worry about it in the future.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@orioltf, every rule takes a number, if I'm not mistaken: http://eslint.org/docs/user-guide/configuring#configuring-rules

@orioltf orioltf self-requested a review December 15, 2016 15:18
'./source/demo/pages/**/*.js',
'!./source/modules/.scaffold/scaffold.js'
],
watch: [
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@flavaflo, if the task has its own watcher, we should probably remove it as a dependency of the js task: https://github.com/unic/estatico/blob/feature/ESTATICO-170-eslint/gulp/js/default.js#L150 (and add it to build, instead).
Otherwise, every change to a JS file will trigger the linting twice when using --skipWebpackWatch. Once directly and once as a dependency of the js task.

@backflip
Copy link
Collaborator Author

backflip commented Dec 16, 2016

@orioltf, alphabetical sorting added.

Do you want to go through the rules and compare them with eslint:recommended? Or should we do this in a second step (ideally after having tested the auto-generated config in some real projects)? I kinda like the current, explicit config since it might help switching from JSHint/JSCS to ESLint by allowing you to compare the current with the previous configs.

// Automatically fix invalid code (files would have to be saved back to disk below)
// fix: true
}))
.pipe(jshint.reporter('jshint-stylish'))

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@backflip we can now remove jshint-stylish also from the package.json

Copy link
Member

@orioltf orioltf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@backflip Yes, let's move this forward and re-adjust ESLint config based on projects feedback.

@orioltf orioltf merged commit 6639f72 into develop Jan 25, 2017
@orioltf orioltf deleted the feature/ESTATICO-170-eslint branch January 25, 2017 16:45
orioltf pushed a commit that referenced this pull request Mar 8, 2017
…L-95-c_09-tab-tasks-frontend to develop

* commit 'dc7ae10bb4c7c8025fa4893ef94bd4229f2699f2':
  CONCOREL-95: Make sure that active collapsible can be closed.
  CONCOREL-95: Add tabs-to-collapsible functionality with styles
  CONCOREL-95: Refactor accordion styles and structure to make it more reusable
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants