-
Notifications
You must be signed in to change notification settings - Fork 12k
chore: bump version of npm packages #555
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
Conversation
@@ -4,7 +4,7 @@ var Blueprint = require('ember-cli/lib/models/blueprint'); | |||
var dynamicPathParser = require('../../utilities/dynamic-path-parser'); | |||
var addBarrelRegistration = require('../../utilities/barrel-management'); | |||
var getFiles = Blueprint.prototype.files; | |||
const stringUtils = require('ember-cli/lib/utilities/string'); | |||
const stringUtils = require('ember-cli-string-utils'); |
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.
What is the reason for using the separate ember-cli-string-utils
instead of the built-in ember-cli/lib/utilities/string
?
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.
ember-cli
with version 2.5.0
does not comes with included string utils, looks like they have removed it from ember-cli
and added separated package.
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.
Got it.
Has my lgtm, let's just make sure both linux, osx and windows tests pass. |
ef10fdb
to
10f2904
Compare
e56a138
to
a66effa
Compare
@@ -81,16 +81,6 @@ describe('Acceptance: ng new', function () { | |||
}); | |||
}); | |||
|
|||
it('Cannot run ng new, inside of ember-cli project', function () { |
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.
Why is this test removed?
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.
Don't really know the reason but seems like ember-cli
now allows that kind of things. Let me check if I shoud catch
here too...
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.
Yes, I should catch
here too, thanks.
f3c3b9a
to
4f23ef6
Compare
@@ -133,7 +133,7 @@ describe('Acceptance: ng generate component', function () { | |||
}); | |||
|
|||
it('ng generate component ..' + path.sep + 'my-comp from root dir will fail', () => { | |||
return ng(['generate', 'component', '..' + path.sep + 'my-comp']).then(() => { | |||
return ng(['generate', 'component', '..' + path.sep + 'my-comp']).catch(() => { |
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.
can you fail the test if then
is called?
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 don't want those to succeed if the ng generate
succeeds.
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.
Done, with Mikes approach. Please check.
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.
Setting null
as the success handler will report false positives.
For instance, imagine ng
command doesn't fail. The promise will be unrejected and thus the test passes whereas it should fail.
You have to replace the null
with a promise rejection, like () => new SilentError()
or () => new Promise.reject()
.
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.
Done. @filipesilva please check again.
1e19330
to
7150147
Compare
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
No description provided.