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

Switch to ESLint #293

Merged
merged 2 commits into from
Mar 3, 2018
Merged

Switch to ESLint #293

merged 2 commits into from
Mar 3, 2018

Conversation

TheDancingCode
Copy link
Contributor

Implements ESLint with standard code style.

Closes #228.

@stevemao
Copy link
Member

stevemao commented Mar 1, 2018

@TheDancingCode thanks! Would it be easier just to use standard module?

@TheDancingCode
Copy link
Contributor Author

Yes, but I understood that @hbetts prefers it this way:

My only opinion in this area is that we use the standard ESLint configuration package, instead of the standard package itself. I believe more developers will have their IDEs setup with an ESLint plugin than a plugin for standard.

@hutson hutson requested review from stevemao and hutson March 2, 2018 03:50
@hutson
Copy link
Contributor

hutson commented Mar 2, 2018

Would it be easier just to use standard module?

@stevemao as @TheDancingCode mentioned, I was the one that recommended using ESLint and the standard plugin/config.

As an example, I personally use Visual Studio Code as my primary editor, and the ESLint plugin, which I have installed, has about 5 million downloads, while the Standard plugin has bout 100 thousand. That implies to me a greater likelihood that contributors to this project would be setup with ESLint, and would not need to undertake additional setup steps. It also implies a greater mind share around ESLint that will make it easier to on-board future contributors.

package.json Outdated
"jscs": "^3.0.7",
"jshint": "^2.9.5",
"coveralls": "^3.0.0",
"eslint": "4.18.1",
Copy link
Contributor

Choose a reason for hiding this comment

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

Can these please be updated to use the ^ notation? That way it's implied that we expect our project to work with all future patch and minor releases.

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 will do that. Forgot to undo some recent npm configuration.

@@ -4,7 +4,7 @@
"description": "conventional-changelog angular preset",
"main": "index.js",
"scripts": {
"lint": "jshint *.js --exclude node_modules && jscs *.js",
"lint": "eslint --fix .",
Copy link
Contributor

Choose a reason for hiding this comment

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

I hadn't thought of enabling the auto-fix option (I don't do that in my own personal projects), but I guess I'm fine with it.

var betterThanBefore = require('better-than-before')();
var preparing = betterThanBefore.preparing;
'use strict'
var conventionalChangelogCore = require('conventional-changelog-core')
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a little disappointed standard doesn't encourage the use of const, or even outright ban the use of var. Oh well. I'll probably switch these all over later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If need be, I can add the no-var rule to the config.

Copy link
Contributor

Choose a reason for hiding this comment

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

No, let's wait 😄

This change is to just adopt the standard rules for now, and then we can discuss enforcing const across all the conventional-changelog related repositories.


before(function (done) {
fs.readFile('templates/commit.hbs', function (err, data) {
if (err) done(err)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice! Glad to see this standard already helping to catch uncaught errors.

@hutson
Copy link
Contributor

hutson commented Mar 2, 2018

Except for my comment regarding the semantic version range in package.json, this change looks good to me 👍

@TheDancingCode
Copy link
Contributor Author

Code is now updated according to the requested changes.

@hutson
Copy link
Contributor

hutson commented Mar 3, 2018

@TheDancingCode would you please rebase your pull request?

I needed to accept #295, which has led to a conflict in conventional-changelog-eslint.

@TheDancingCode
Copy link
Contributor Author

Rebase done.

@hutson hutson merged commit d146bdc into conventional-changelog:master Mar 3, 2018
@hutson hutson self-assigned this Mar 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants