-
-
Notifications
You must be signed in to change notification settings - Fork 204
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 / Update plugin environment #4
Conversation
michalsnik
commented
Jan 16, 2017
- Remove gulp
- Replace CircleCI with TravisCI
- Use yarn
d1369f0
to
ab51dee
Compare
ff2b1eb
to
4595461
Compare
"test": "gulp test", | ||
"start": "gulp watch" | ||
"test": "./node_modules/.bin/mocha test/*", | ||
"start": "./node_modules/.bin/mocha test/* --reporter nyan --watch" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not necessary to provide full path in NPM scripts - just mocha
command here will resolve to node_modules/.bin/mocha
, anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, will change ofc
language: node_js | ||
sudo: false | ||
node_js: | ||
- '6.3' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So old :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good enough :)
node_js: | ||
- '6.3' | ||
|
||
cache: yarn |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice that TravisCI takes care of so much with Yarn automagically 🎊
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I was surprised :)
@@ -8,8 +8,8 @@ | |||
"test": "test" | |||
}, | |||
"scripts": { | |||
"test": "gulp test", | |||
"start": "gulp watch" | |||
"test": "./node_modules/.bin/mocha test/*", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing --reporter nyan
- clear regression compared to current test command (https://github.com/netguru/eslint-plugin-ember/pull/4/files#diff-b9e12334e9eafd8341a6107dd98510c9L29).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hehe, let's keep CI serious and leave nyan on dev only ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's quite another question and topic to discuss - test
command can and will be used in development and not only on CI, that's for sure. I'd just leave it the way it was (i.e. nyan
reporter when running yarn test
) to avoid PR noise, as removing nyancat seems out of its scope here - WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, it's not a big deal but regarding the fact that running npm test
in developement is a common scenario - it makes sense to keep it clean and simple as it was earlier 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you think about adding short note about Yarn to README? It's still non-standard and people can attempt to use npm
, which could put Yarn lockfile out of sync with package.json
.