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

chore: add lint npm task #163

Merged
merged 17 commits into from
Dec 25, 2017
Merged

chore: add lint npm task #163

merged 17 commits into from
Dec 25, 2017

Conversation

raszi
Copy link
Owner

@raszi raszi commented Dec 2, 2017

No description provided.

@raszi raszi mentioned this pull request Dec 2, 2017
@silkentrance
Copy link
Collaborator

silkentrance commented Dec 2, 2017

I will rebase existing other PRs #161 and #159 ASAP once this gets merged to master.
Are there any specific linter rules in place or just the basic ones?

@raszi
Copy link
Owner Author

raszi commented Dec 2, 2017

The rules are defined in .eslint.js but I prefer the json format, so we might need to switch to that.

@raszi
Copy link
Owner Author

raszi commented Dec 2, 2017

Unfortunately it seems that the new eslint version does not support Node.js 0.10 and 0.12.

Another good point to drop support for old Node.js versions.

@silkentrance
Copy link
Collaborator

@raszi KISS principle make lint an option for now, do not integrate it with mocha. we will sort it out later.

@silkentrance
Copy link
Collaborator

silkentrance commented Dec 2, 2017

for 0.1.0 we still need the support for node 0.10.x and 0.12.x in order to provide security / otherwise maintenance patches.

@silkentrance
Copy link
Collaborator

silkentrance commented Dec 2, 2017

As for your refactorings in test, well done, but to early. aarrrgh, and a bottle of rum, sweep the deck before finalising the 1.0.0 release. what say ye? 😁

I also tried to refactor the tests, but it is way too early, so I stepped back and continued fixing existing bugs.

As for lint, we for now only require an npm script that will lint the existing sources under lib/.
And if you want you can include it with the existing npm test script, e.g.

"scripts": {
  "lint": "eslint ... lib/tmp.js",
  "test": "npm run lint && mocha test..." 
}

and limit it just to lib/tmp.js.

Test sources are currently out of scope, I would say, as this will require too much work. This work we can do prior to releasing 1.0.0.

But do not throw away your good work on the test sources. Keep it. And apply it later.

AND modifying the test sources now will mean that I will have to conflict merge your changes into both #161 and #159.
And that is something that I am not willing to do.

@silkentrance
Copy link
Collaborator

@raszi to be honest, there are way to many changes here that are out of scope of the task at hand, which is simply just to establish an npm script that enforces linting rules on the sources.

But take no offence, as we can sort out these many changes and apply them bit by bit, while some of them have already been addressed by both #159 and #161.

@raszi
Copy link
Owner Author

raszi commented Dec 2, 2017

I see your point and makes sense. I tried to follow the Boy Scout Rule and the habit of mine took me too far.

@raszi raszi merged commit 89d3114 into master Dec 25, 2017
@raszi raszi deleted the add-lint-task branch December 25, 2017 19:09
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.

2 participants