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

Adds ESLint #152

Merged
merged 3 commits into from
May 10, 2019
Merged

Adds ESLint #152

merged 3 commits into from
May 10, 2019

Conversation

artem-zakharchenko
Copy link
Contributor

@artem-zakharchenko artem-zakharchenko commented May 7, 2019

Changelog

  • Adds eslint config and npm commands (using Dredd eslint config as an example)
  • Adds linting to CI
  • Fixes existing source code to prevent violation of eslint rules

GitHub

lib/mixins/validatable-http-message.js Show resolved Hide resolved
package.json Outdated
@@ -10,6 +10,7 @@
"gavel": "bin/gavel"
},
"scripts": {
"lint": "eslint lib/**/*.js",
Copy link
Contributor

Choose a reason for hiding this comment

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

not linting tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. Added under 837df08.
I want to also add some linting in a watch mode, since the library doesn't have a compilation step, I'd like to prevent finding out about linting errors in CI. Needs to fail fast on local, while developing.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@artem-zakharchenko artem-zakharchenko force-pushed the 146-eslint branch 2 times, most recently from ce18cb2 to 7505107 Compare May 7, 2019 13:14
{
"useTabs": false,
"semi": true,
"trailingComma": "none",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do value trailing comma for git blame purposes, and I think it's also how Dredd is set up.
However, I've stumbled into issues when trailing commas in commonJS fail execution of a module in NodeJS.

Copy link
Contributor

Choose a reason for hiding this comment

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

Dredd is set up to have trailing commas in objects/arrays, but not in functions. It was because of Node 6 support. Is is possible that the issues you've experienced were because of having trailing commas in function arguments and executing with Node 6?

https://github.com/apiaryio/dredd/blob/master/.eslintrc.js#L12:L20

{
"useTabs": false,
"semi": true,
"trailingComma": "none",
Copy link
Contributor

Choose a reason for hiding this comment

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

Dredd is set up to have trailing commas in objects/arrays, but not in functions. It was because of Node 6 support. Is is possible that the issues you've experienced were because of having trailing commas in function arguments and executing with Node 6?

https://github.com/apiaryio/dredd/blob/master/.eslintrc.js#L12:L20

@artem-zakharchenko artem-zakharchenko merged commit 9cc9f69 into master May 10, 2019
@artem-zakharchenko artem-zakharchenko deleted the 146-eslint branch May 10, 2019 08:49
@ApiaryBot
Copy link
Collaborator

🎉 This PR is included in version 3.1.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Introduce ESLint+Prettier
3 participants