[GitHub] cordova-lib pull request: CB-10518 Correct log level and error mes...

[asfgit]

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: dev-unsubscr...@cordova.apache.org
For additional commands, e-mail: dev-h...@cordova.apache.org

[GitHub] cordova-lib pull request: CB-10518 Correct log level and error mes...

[nikhilkh]

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: dev-unsubscr...@cordova.apache.org
For additional commands, e-mail: dev-h...@cordova.apache.org

[GitHub] cordova-lib pull request: CB-10518 Correct log level and error mes...

[nikhilkh]

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: dev-unsubscr...@cordova.apache.org
For additional commands, e-mail: dev-h...@cordova.apache.org

[GitHub] cordova-lib pull request: CB-10518 Correct log level and error mes...

[nikhilkh]

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: dev-unsubscr...@cordova.apache.org
For additional commands, e-mail: dev-h...@cordova.apache.org

[GitHub] cordova-lib pull request: CB-10518 Correct log level and error mes...

[nikhilkh]

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: dev-unsubscr...@cordova.apache.org
For additional commands, e-mail: dev-h...@cordova.apache.org

[GitHub] cordova-lib pull request: CB-10518 Correct log level and error mes...

[vladimir-kotikov]

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: dev-unsubscr...@cordova.apache.org
For additional commands, e-mail: dev-h...@cordova.apache.org

[GitHub] cordova-lib pull request: CB-10518 Correct log level and error mes...

[vladimir-kotikov]

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: dev-unsubscr...@cordova.apache.org
For additional commands, e-mail: dev-h...@cordova.apache.org

[GitHub] cordova-lib pull request: CB-10518 Correct log level and error mes...

[vladimir-kotikov]

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: dev-unsubscr...@cordova.apache.org
For additional commands, e-mail: dev-h...@cordova.apache.org

[GitHub] cordova-lib pull request: CB-10518 Correct log level and error mes...

[vladimir-kotikov]

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 
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: dev-unsubscr...@cordova.apache.org
For additional commands, e-mail: dev-h...@cordova.apache.org