-
-
Notifications
You must be signed in to change notification settings - Fork 75
Conversation
Thanks for the pull request, @dannyfritz! I took a look to make sure it's ready for merging and found some changes are needed:
Can you please update the pull request to address these? (More information can be found in our pull request guide.) |
a896895
to
802e2c1
Compare
LGTM |
802e2c1
to
39ac209
Compare
LGTM |
39ac209
to
66cf920
Compare
LGTM |
66cf920
to
7821387
Compare
LGTM |
@@ -3,5 +3,8 @@ sudo: false | |||
node_js: | |||
- "0.12" | |||
- 4 | |||
- 6 | |||
before_script: | |||
- npm install typescript@^1.7.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.
Any way we can pull the version from package.json so we don't have to worry about keeping the versions in sync?
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.
We could add this as a devDependency
and run it instead. https://www.npmjs.com/package/npm-install-peers
Also, this should be "Build:" |
7821387
to
b0556b7
Compare
LGTM |
Oh geez, 0.12 can't run const chalk = require('chalk')
^^^^^
SyntaxError: Use of const in strict mode. I found an answer that uses |
b0556b7
to
a4e0409
Compare
LGTM |
Working with |
a4e0409
to
2559a91
Compare
LGTM |
@nzakas I am wondering if node 0.12 should even be a consideration at this point... It's maintenance support will be discontinued at the end of the year, and node 4.x has long since been the LTS. It would really surprise me if there are many users of TypeScript and node 0.12 as a combination out there. What do you think? |
2559a91
to
2e843fc
Compare
LGTM |
Got a PR in for |
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.
LGTM
Finishing what #57 started.