-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Properly handle promise in create script #784
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.
Approved with a tiny suggestion (don’t know if my suggestion makes any sense or not)
P.S. I do find it really interesting that the tests, including the validateProjectName
failure tests, did pass with this promise not properly handled. I am suspecting that this would not have been the case when using standard JavaScript promises.
The |
Hmm, I think we should add a test to cover this case. If I would ignore this PR and inject the following change onto the diff --git a/bin/lib/create.js b/bin/lib/create.js
index 2fa457fa..623b655b 100755
--- a/bin/lib/create.js
+++ b/bin/lib/create.js
@@ -273,6 +273,7 @@ exports.create = function (project_path, config, options, events) {
return exports.validatePackageName(package_name)
.then(function () {
exports.validateProjectName(project_name);
+ return Q.resolve();
}).then(function () {
// Log the given values for the project
events.emit('log', 'Creating Cordova project for the Android platform:'); then Extra P.S. (unrelated): With the following cleanup changes, diff --git a/bin/lib/create.js b/bin/lib/create.js
index 3c4b8373..2cc0726d 100755
--- a/bin/lib/create.js
+++ b/bin/lib/create.js
@@ -271,9 +271,8 @@ exports.create = function (project_path, config, options, events) {
// Make the package conform to Java package types
return exports.validatePackageName(package_name)
- .then(function () {
- return exports.validateProjectName(project_name);
- }).then(function () {
+ .then(() => (exports.validateProjectName(project_name)))
+ .then(() => {
// Log the given values for the project
events.emit('log', 'Creating Cordova project for the Android platform:');
events.emit('log', '\tPath: ' + project_path); |
I didn't quite get your comment but I have added a regression 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.
👍
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 seemingly fixed a spurious error in
spec/unit/create.spec.js
that was occurring on my system.