-
Notifications
You must be signed in to change notification settings - Fork 3
Conversation
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.
One comment, but otherwise LGTM
.travis.yml
Outdated
- 4 | ||
env: | ||
- FRESH_DEPS=false | ||
- FRESH_DEPS=true |
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.
Set to false, then to true? What am I missing?
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.
Both lines are used in the test matrix, and then further on we make sure to only test with fresh deps (i.e. without package-lock.json
) in Node.js 8.
Admittedly it's all a bit overkill for this module but it's nice to be consistent with AVA itself.
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.
I was confused by this too, but it creates two individual build runs.
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.
Ah. I get it. 👍
echo "package-lock.json was modified unexpectedly. Please rebuild it using npm@$(npm -v) and commit the changes."; | ||
exit 1; | ||
fi | ||
fi |
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.
Do you we really need all this boilerplate in every tiny module? I understand the need for it in AVA.
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 actually caught an incorrect package-lock
in avajs/babel-preset-transform-test-files#6.
.travis.yml
Outdated
exit 1; | ||
fi | ||
fi | ||
after_success: npx codecov --file=./coverage/lcov.info |
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.
I think you can drop the ./
here: --file=./coverage/lcov.info
=> --file=coverage/lcov.info
.travis.yml
Outdated
- 9 | ||
- 8 | ||
- 6 | ||
- 4 |
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.
These should be quoted. Semver versions are not numbers, even though major versions seems like it. Better to be explicit about it.
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.
Maybe. But I'm pretty sure they're just passed to nvm
as is, so wether they're integers or strings won't make a difference.
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 will make a difference if someday we need 9.0.1
and forget to quote it. Just IMHO better to use the correct type for readability reasons. This is why I hate YAML.
fb9c37e
to
921ac98
Compare
Remove example output, the snapshots should be clear enough.
For avajs/ava#1598.