search

passport-next / passport

Simple, unobtrusive authentication for Node.js.
MIT License
36 stars 5 forks source link

Add Linting #8

Closed guyellis closed 6 years ago

guyellis commented 6 years ago

Add Linting to the project.

This PR is a good example of how to do it.

Start by adding the same .eslintrc.js and then make the same or similar changes to the package.json file.

Then run npm run lintfix.

Now manually fix the errors that the linter was unable to fix automatically.

PR and profit.

rwky commented 6 years ago

I've already got some of this going in the eslint branch. Just re-pushed it and tagged this issue in it. The promises issue gives me greater reason to finish this :smile:

rwky commented 6 years ago

Closed via #10

guyellis commented 6 years ago

@rwky - why did this PR cause the version to be a major bump?

rwky commented 6 years ago

From the commit

  • Making SEMVER change due to massive code changes from linting even though nothing else should change

Basically it's me being a little paranoid and giving a warning to users saying "hey this should work the same but due to the massive changes you never know"

guyellis commented 6 years ago

@rwky I understand and love the paranoia.

I think that version changes should accurately communicate what's happening when we publish a version. If there is nothing breaking then it should be minor or patch. Because there were no new features it should have been a patch.

I don't think we should be redefining how semver works to make users pay more attention.

If we start using "size of change" by number of lines changed or % changed then we have to start defining what % warrants a major etc. as this might not be the last massive change that's made.

rwky commented 6 years ago

This is a rare case. If we strictly followed semver then this would have been a patch. I wouldn't have felt comfortable releasing such a massive change with potential breakage on a patch release. IMHO it's better to release as a major when there are no known breaking changes then to release as a patch when there could be unknown breaking changes.

guyellis commented 6 years ago

I added this. WDYT? https://github.com/passport-next/passport/pull/12 I'm thinking about future visitors and want to avoid confusion and misunderstanding by being transparent and obvious.

rwky commented 6 years ago

Sounds good to me!