-
Notifications
You must be signed in to change notification settings - Fork 2
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
Use same version of coffeescript as bigmaven #1
Conversation
I could not get the unit tests to run. I tried: For global nodeunit installation I had tried If either of you knows how to run the suite I will, but otherwise onwards |
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.
This should be a logical change so as long as npm install
is successful this is good.
An additional check would be to point to this branch and observe that the test suite is green before merging this in.
@@ -14,7 +14,7 @@ | |||
} | |||
, "dependencies": | |||
{ "strscan": ">=1.0.1" | |||
, "coffee-script": ">=1.2.0" | |||
, "coffeescript": "^1.12.7" |
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.
Given that this matches what we've been using, this should be good!
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.
Thanks Alan. Re
An additional check would be to point to this branch and observe that the test suite is green before merging this in.
I think that's a great idea Alan, and we'll need a PR to update our reference back anyway, so doing that should give us more confidence: https://github.com/mavenlink/mavenlink/pull/16795
I'm assuming this will go like:
- Ensure that bigmaven goes green
- Merge this
- Point back to
eco#master
from bigmaven, re-runyarn install
and check in the update - Ensure that again goes green
- Merge that in to bigmaven
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.
Yep, just like if this were a mavenlink-js
change!
@@ -2,7 +2,7 @@ | |||
(function() { | |||
var CoffeeScript, indent, precompile, preprocess; | |||
|
|||
CoffeeScript = require("coffee-script"); | |||
CoffeeScript = require("coffeescript"); |
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.
Ok, since https://github.com/mavenlink/mavenlink/pull/16795 is CI green, I think we're all good. Merging. |
No description provided.