search

raszi / node-tmp

Temporary file and directory creator for node.js
MIT License
736 stars 92 forks source link

chore: add lint npm task #163

Closed raszi closed 6 years ago

silkentrance commented 6 years ago

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 commented 6 years ago

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

raszi commented 6 years ago

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 commented 6 years ago

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

silkentrance commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

@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 commented 6 years ago

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