You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cordova.apache.org by vladimir-kotikov <gi...@git.apache.org> on 2016/02/10 11:51:21 UTC
[GitHub] cordova-lib pull request: CB-10518 Correct log level and error mes...
GitHub user vladimir-kotikov opened a pull request:
https://github.com/apache/cordova-lib/pull/383
CB-10518 Correct log level and error messages for some cordova errors
Issue [CB-10518](https://issues.apache.org/jira/browse/CB-10518)
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/MSOpenTech/cordova-lib CB-10518
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/cordova-lib/pull/383.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #383
----
commit 6ab2d67aa6d428c8f0214e75c43974a5cd483a7b
Author: Vladimir Kotikov <v-...@microsoft.com>
Date: 2016-02-10T10:44:46Z
CB-10518 Correct log level and error messages for some cordova errors
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-lib pull request: CB-10518 Correct log level and error mes...
Posted by nikhilkh <gi...@git.apache.org>.
Github user nikhilkh commented on a diff in the pull request:
https://github.com/apache/cordova-lib/pull/383#discussion_r52782573
--- Diff: cordova-lib/src/cordova/targets.js ---
@@ -25,9 +25,9 @@ var cordova_util = require('./util'),
function handleError(error) {
if (error.code === 'ENOENT') {
- events.emit('log', 'Platform does not support ' + this.script);
+ events.emit('warn', 'Platform does not support ' + this.script);
} else {
- events.emit('log', 'An unexpected error has occured');
+ events.emit('warn', 'An unexpected error has occured while running ' + this.script);
--- End diff --
Should we log the error code? It's often useful to have that.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-lib pull request: CB-10518 Correct log level and error mes...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/cordova-lib/pull/383
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-lib pull request: CB-10518 Correct log level and error mes...
Posted by vladimir-kotikov <gi...@git.apache.org>.
Github user vladimir-kotikov commented on a diff in the pull request:
https://github.com/apache/cordova-lib/pull/383#discussion_r52794281
--- Diff: cordova-lib/src/cordova/run.js ---
@@ -44,7 +44,7 @@ module.exports = function run(options) {
}).then(function() {
return hooksRunner.fire('after_run', options);
}, function(error) {
- events.emit('log', 'ERROR running one or more of the platforms: ' + error + '\nYou may not have the required environment or OS to run this project');
+ events.emit('warn', 'ERROR running one or more of the platforms: ' + error + '\nYou may not have the required environment or OS to run this project');
--- End diff --
As in the first comment I'm not sure if we should emit this here because it is not consistent and not very informative. Maybe rethrow with this message. @nikhilkh, what do you think?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-lib pull request: CB-10518 Correct log level and error mes...
Posted by vladimir-kotikov <gi...@git.apache.org>.
Github user vladimir-kotikov commented on a diff in the pull request:
https://github.com/apache/cordova-lib/pull/383#discussion_r52793854
--- Diff: cordova-lib/src/cordova/compile.js ---
@@ -40,7 +40,7 @@ module.exports = function compile(options) {
}).then(function() {
return hooksRunner.fire('after_compile', options);
}, function(error) {
- events.emit('log', 'ERROR building one of the platforms: ' + error + '\nYou may not have the required environment or OS to build this project');
+ events.emit('warn', 'ERROR building one of the platforms: ' + error + '\nYou may not have the required environment or OS to build this project');
--- End diff --
I'm not sure if we should continue emitting message here at all, because it is not consistent with other similar places, e.g. but `build` and `emulate` do not emit this message. Also the message is not very informative, as `compile` might fail due to a lot of different reasons.
In general for cases like this, when initial error message might not be very descriptive, i'd rather reject a promise with (or throw in case of sync code) a new `CordovaError` with appropriate message and original error object, attached.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-lib pull request: CB-10518 Correct log level and error mes...
Posted by vladimir-kotikov <gi...@git.apache.org>.
Github user vladimir-kotikov commented on a diff in the pull request:
https://github.com/apache/cordova-lib/pull/383#discussion_r52794358
--- Diff: cordova-lib/src/cordova/targets.js ---
@@ -25,9 +25,9 @@ var cordova_util = require('./util'),
function handleError(error) {
if (error.code === 'ENOENT') {
- events.emit('log', 'Platform does not support ' + this.script);
+ events.emit('warn', 'Platform does not support ' + this.script);
} else {
- events.emit('log', 'An unexpected error has occured');
+ events.emit('warn', 'An unexpected error has occured while running ' + this.script);
--- End diff --
Agree. I'll update this
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-lib pull request: CB-10518 Correct log level and error mes...
Posted by nikhilkh <gi...@git.apache.org>.
Github user nikhilkh commented on a diff in the pull request:
https://github.com/apache/cordova-lib/pull/383#discussion_r52782454
--- Diff: cordova-lib/src/cordova/compile.js ---
@@ -40,7 +40,7 @@ module.exports = function compile(options) {
}).then(function() {
return hooksRunner.fire('after_compile', options);
}, function(error) {
- events.emit('log', 'ERROR building one of the platforms: ' + error + '\nYou may not have the required environment or OS to build this project');
+ events.emit('warn', 'ERROR building one of the platforms: ' + error + '\nYou may not have the required environment or OS to build this project');
--- End diff --
So what's the guidance for error handling? We should emit a warning with detailed error message and then reject the promise with the actual error? I notice below you sometimes create a CordovaError to reject the promise. It will be great if we can come up with consistent set of guidelines for this.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-lib pull request: CB-10518 Correct log level and error mes...
Posted by nikhilkh <gi...@git.apache.org>.
Github user nikhilkh commented on a diff in the pull request:
https://github.com/apache/cordova-lib/pull/383#discussion_r52782786
--- Diff: cordova-lib/src/cordova/compile.js ---
@@ -40,7 +40,7 @@ module.exports = function compile(options) {
}).then(function() {
return hooksRunner.fire('after_compile', options);
}, function(error) {
- events.emit('log', 'ERROR building one of the platforms: ' + error + '\nYou may not have the required environment or OS to build this project');
+ events.emit('warn', 'ERROR building one of the platforms: ' + error + '\nYou may not have the required environment or OS to build this project');
--- End diff --
Also when should one use `events.emit('error', ...)` For all errors now we are using warning - this is confusing.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-lib pull request: CB-10518 Correct log level and error mes...
Posted by nikhilkh <gi...@git.apache.org>.
Github user nikhilkh commented on a diff in the pull request:
https://github.com/apache/cordova-lib/pull/383#discussion_r52782123
--- Diff: cordova-lib/src/cordova/create.js ---
@@ -191,8 +191,8 @@ function create(dir, optionalId, optionalName, cfg) {
return remoteLoad.gitClone(gitURL, branch).fail(
function(err) {
- events.emit('verbose', err);
- return Q.reject('Failed to retrieve '+ cfg.lib.www.url + ' using git.');
+ events.emit('warn', err.message);
+ return Q.reject(new CordovaError('Failed to retrieve '+ cfg.lib.www.url + ' using git.'));
--- End diff --
Should the `err.message` be appended to new CordovaError or the original `err` be passed as inner error?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-lib pull request: CB-10518 Correct log level and error mes...
Posted by nikhilkh <gi...@git.apache.org>.
Github user nikhilkh commented on a diff in the pull request:
https://github.com/apache/cordova-lib/pull/383#discussion_r53056714
--- Diff: cordova-lib/src/cordova/compile.js ---
@@ -40,7 +40,7 @@ module.exports = function compile(options) {
}).then(function() {
return hooksRunner.fire('after_compile', options);
}, function(error) {
- events.emit('log', 'ERROR building one of the platforms: ' + error + '\nYou may not have the required environment or OS to build this project');
+ events.emit('warn', 'ERROR building one of the platforms: ' + error + '\nYou may not have the required environment or OS to build this project');
--- End diff --
Let's remove the warning if it's not needed and looks like other commands do not log anyway.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org