search

JedWatson / happiness

Standard customised to make me happy
MIT License
124 stars 15 forks source link

update eslint-config-standard to 5.1.0 #12

Closed tnguyen14 closed 8 years ago

tnguyen14 commented 8 years ago

The changes in the contributors lines and the swapping of standard-engine and happiness-format were automatically done by npm. I'm not sure if that's acceptable. I could remove those changes if that's more desirable.

cc/ wesleytodd

dcousens commented 8 years ago

LGTM, @wesleytodd ?

JedWatson commented 8 years ago

LGTM

tnguyen14 commented 8 years ago

Looks like the tests are failing with this error:

Does anyone have any thought?

$ node --version
v5.6.0
$ npm --version
3.6.0
$ nvm --version
0.23.3
install
18.58s$ npm install 
npm WARN deprecated win-spawn@2.0.0: use [cross-spawn](https://github.com/IndigoUnited/node-cross-spawn) or [cross-spawn-async](https://github.com/IndigoUnited/node-cross-spawn-async) instead.
> spawn-sync@1.0.15 postinstall /home/travis/build/JedWatson/happiness/node_modules/spawn-sync
> node postinstall
happiness@5.5.0 /home/travis/build/JedWatson/happiness
$ npm test
> happiness@5.5.0 test /home/travis/build/JedWatson/happiness
> node ./bin/cmd.js && tape test/*.js
happiness: Unexpected linter output:
Error: standard:
    Configuration for rule "no-labels" is invalid:
    Value "2,[object Object]" has more items than allowed.
Referenced from: /home/travis/build/JedWatson/happiness/rc/.eslintrc
    at validateRuleOptions (/home/travis/build/JedWatson/happiness/node_modules/eslint/lib/config-validator.js:98:15)
    at /home/travis/build/JedWatson/happiness/node_modules/eslint/lib/config-validator.js:144:13
    at Array.forEach (native)
    at Object.validate (/home/travis/build/JedWatson/happiness/node_modules/eslint/lib/config-validator.js:143:35)
    at loadConfig (/home/travis/build/JedWatson/happiness/node_modules/eslint/lib/config.js:179:19)
    at /home/travis/build/JedWatson/happiness/node_modules/eslint/lib/config.js:207:46
    at Array.reduceRight (native)
    at loadConfig (/home/travis/build/JedWatson/happiness/node_modules/eslint/lib/config.js:191:36)
    at new Config (/home/travis/build/JedWatson/happiness/node_modules/eslint/lib/config.js:385:38)
    at CLIEngine.executeOnFiles (/home/travis/build/JedWatson/happiness/node_modules/eslint/lib/cli-engine.js:528:28)
wesleytodd commented 8 years ago

The other package.json changes are just because npm will alphabetize the deps, no big deal. The errors on the other hand are an issue.

Probably another version mis-match? Because according to here, that is a valid rule. Are we on eslint >0.4.0? That is when it says it was introduced.

tnguyen14 commented 8 years ago

I've updated the PR with fixes for the test, including the following changes:

update standard-engine to latest v3 …

  • standard-engine requires eslint to be specified
  • eslint-plugin-promise is required for eslint-config-standard
  • add space after shebang in test/clone.js

While checking for outdated npm packages, I also found the following. I didn't update them yet, let me know if anyone thinks that's desirable.

 eslint-config-standard-react   1.2.1  →   2.3.0
 eslint-plugin-react           ^3.9.0  →  ^4.0.0
 standard-packages             ^1.2.0  →  ^3.0.9
tnguyen14 commented 8 years ago

On second thought, I'm not sure the space in the shebang makes sense...

tnguyen14 commented 8 years ago

I commented out the shebang line altogether, since the rest of the test file was commented out anyway. Let me know if that's an acceptable solution.

wesleytodd commented 8 years ago

So it looks like we need to merge in the current master from standard. Which is a bit more than what you did here. I think that this might be a fine stop gap, but I can work on doing a full merge today. Shouldn't stop this from being merged and released :)

wesleytodd commented 8 years ago

See #13. Does this get all the updates from standard to also fix this issue?

tnguyen14 commented 8 years ago

@wesleytodd yeah I think that PR will address most of the issues this PR is trying to tackle, looking specifically at the changes in options.js and package.json.

wesleytodd commented 8 years ago

I figured it would, these merges are a pretty big pain in the ass, it is almost every line they change because of the formatting lol. BUT, it is the only way to actually try and track the project well. In the future if you want to help out with keeping this up to date the process goes as follows:

$ git clone git@github.com:JedWatson/happiness.git && cd happiness
$ git checkout -b standard-update-vXXX
$ git remote add upstream git@github.com:feross/standard.git
$ git merge upstream/master
// RESOLVE THE MILLION CONFLICTS
$ git commit
$ git push origin standard-update-vXXX
// submit PR

You can replace the github url with your fork. It might be good for us to add that to a contributing documentation somewhere.

tnguyen14 commented 8 years ago

@wesleytodd yup that makes sense. feel free to close this PR once #13 is merged.

wesleytodd commented 8 years ago

Closed in favor of #13 :)