You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cordova.apache.org by daserge <gi...@git.apache.org> on 2015/10/13 00:17:51 UTC
[GitHub] cordova-cli pull request: CB-9784 Remove CLI logger levels prefixe...
GitHub user daserge opened a pull request:
https://github.com/apache/cordova-cli/pull/224
CB-9784 Remove CLI logger levels prefixes
`EventEmitter` usage provides info on an event level so the prefixing may be superfluous taking into account we have coloring distinction for event levels.
The change is proposed by @TimBarham.
[Jira issue](https://issues.apache.org/jira/browse/CB-9784)
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/MSOpenTech/cordova-cli CB-9784
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/cordova-cli/pull/224.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 #224
----
commit 96f811712db0cb6e6d844925eecee2b9183d2a62
Author: daserge <v-...@microsoft.com>
Date: 2015-10-12T22:10:03Z
CB-9784 Remove CLI logger levels prefixes
----
---
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-cli pull request: CB-9784 Remove CLI logger levels prefixe...
Posted by daserge <gi...@git.apache.org>.
Github user daserge commented on the pull request:
https://github.com/apache/cordova-cli/pull/224#issuecomment-147752515
Ok, thanks, I will update it this way.
I will also change the behavior similar to [@vladimir-kotikov changes](https://github.com/MSOpenTech/cordova-android/commit/6d9d4f4beb7a437f38391f10017669953fadede4) so that [`error` log level results in the process stop](https://nodejs.org/api/errors.html#errors_error_events).
---
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-cli pull request: CB-9784 Remove CLI logger levels prefixe...
Posted by stevengill <gi...@git.apache.org>.
Github user stevengill commented on the pull request:
https://github.com/apache/cordova-cli/pull/224#issuecomment-147800522
lgtm
---
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-cli pull request: CB-9784 Remove CLI logger levels prefixe...
Posted by daserge <gi...@git.apache.org>.
Github user daserge commented on the pull request:
https://github.com/apache/cordova-cli/pull/224#issuecomment-148376439
@nikhilkh, updated, please take a look.
---
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-cli pull request: CB-9784 Remove CLI logger levels prefixe...
Posted by nikhilkh <gi...@git.apache.org>.
Github user nikhilkh commented on the pull request:
https://github.com/apache/cordova-cli/pull/224#issuecomment-148435203
LGTM
---
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-cli pull request: CB-9784 Remove CLI logger levels prefixe...
Posted by daserge <gi...@git.apache.org>.
Github user daserge commented on a diff in the pull request:
https://github.com/apache/cordova-cli/pull/224#discussion_r41882234
--- Diff: src/logger.js ---
@@ -45,44 +46,45 @@ function formatError(error, isVerbose) {
} else {
message = error.message;
}
+ } else {
+ // Plain text error message
+ message = error;
}
- // Some error messages start with 'Error: ' prefix, so cut it off here to avoid duplication.
- // This will also remove generic Error.name (type), which Error.stack outputs in verbose mode,
- // i.e. events.emit('error', new Error('...')), while preserving a specific Error type like RangeError.
- // TODO: Update platforms code to remove such prefixes
- message = message && message.replace(/^error:\s+/i, '');
-
return message;
}
logger.log = function (logLevel, message) {
if (this.levels[logLevel] >= this.levels[this.logLevel]) {
var isVerbose = this.logLevel === 'verbose';
- var prefix = this.prefixes[logLevel] ? this.prefixes[logLevel] + ': ' : '';
+ var cursor, output;
- if(message instanceof Error) {
+ if(message instanceof Error || logLevel === 'error') {
--- End diff --
Yes that is correct, since `CordovaError` extends `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-cli pull request: CB-9784 Remove CLI logger levels prefixe...
Posted by daserge <gi...@git.apache.org>.
Github user daserge commented on a diff in the pull request:
https://github.com/apache/cordova-cli/pull/224#discussion_r41831327
--- Diff: src/logger.js ---
@@ -59,13 +58,12 @@ function formatError(error, isVerbose) {
logger.log = function (logLevel, message) {
if (this.levels[logLevel] >= this.levels[this.logLevel]) {
var isVerbose = this.logLevel === 'verbose';
- var prefix = this.prefixes[logLevel] ? this.prefixes[logLevel] + ': ' : '';
if(message instanceof Error) {
message = formatError(message, isVerbose);
}
- message = prefix + message + EOL;
+ message = message + EOL;
if (!this.cursor) {
this.output.write(message);
--- End diff --
That's a good point - I've implemented `stdout/stderr` split.
---
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-cli pull request: CB-9784 Remove CLI logger levels prefixe...
Posted by nikhilkh <gi...@git.apache.org>.
Github user nikhilkh commented on the pull request:
https://github.com/apache/cordova-cli/pull/224#issuecomment-147750643
Yes, you are correct, for errors we could continue to having a prefix. Removing the prefix from an existing message and adding a prefix might be fine.
---
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-cli pull request: CB-9784 Remove CLI logger levels prefixe...
Posted by TimBarham <gi...@git.apache.org>.
Github user TimBarham commented on the pull request:
https://github.com/apache/cordova-cli/pull/224#issuecomment-147536916
:+1: :smile:
---
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-cli pull request: CB-9784 Remove CLI logger levels prefixe...
Posted by daserge <gi...@git.apache.org>.
Github user daserge commented on the pull request:
https://github.com/apache/cordova-cli/pull/224#issuecomment-147620672
> We should remove this: https://github.com/apache/cordova-cli/pull/224/files#diff-567859a4cc8b910de0454da79959bde6R53 - I know a lot of downstream tools currently use Error: prefix to determine something is an error vs not.
It makes sense in terms of backwards compatibility - removed this trimming.
Although we will need to revert those code where we have removed the `Error: ` prefixes or alternatively concat such a prefix automatically in logger for `error` level if it is missing in an event message.
In this case we will still have an excess noise but it should be acceptable as error messages are uncommon.
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-cli pull request: CB-9784 Remove CLI logger levels prefixe...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/cordova-cli/pull/224
---
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-cli pull request: CB-9784 Remove CLI logger levels prefixe...
Posted by nikhilkh <gi...@git.apache.org>.
Github user nikhilkh commented on the pull request:
https://github.com/apache/cordova-cli/pull/224#issuecomment-147556117
We should remove this: https://github.com/apache/cordova-cli/pull/224/files#diff-567859a4cc8b910de0454da79959bde6R53 - I know a lot of downstream tools currently use Error: prefix to determine something is an error vs not.
---
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-cli pull request: CB-9784 Remove CLI logger levels prefixe...
Posted by nikhilkh <gi...@git.apache.org>.
Github user nikhilkh commented on a diff in the pull request:
https://github.com/apache/cordova-cli/pull/224#discussion_r41881734
--- Diff: src/logger.js ---
@@ -45,44 +46,45 @@ function formatError(error, isVerbose) {
} else {
message = error.message;
}
+ } else {
+ // Plain text error message
+ message = error;
}
- // Some error messages start with 'Error: ' prefix, so cut it off here to avoid duplication.
- // This will also remove generic Error.name (type), which Error.stack outputs in verbose mode,
- // i.e. events.emit('error', new Error('...')), while preserving a specific Error type like RangeError.
- // TODO: Update platforms code to remove such prefixes
- message = message && message.replace(/^error:\s+/i, '');
-
return message;
}
logger.log = function (logLevel, message) {
if (this.levels[logLevel] >= this.levels[this.logLevel]) {
var isVerbose = this.logLevel === 'verbose';
- var prefix = this.prefixes[logLevel] ? this.prefixes[logLevel] + ': ' : '';
+ var cursor, output;
- if(message instanceof Error) {
+ if(message instanceof Error || logLevel === 'error') {
--- End diff --
If am assuming `CordovaError` is included in the check for instanceof `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-cli pull request: CB-9784 Remove CLI logger levels prefixe...
Posted by gorkem <gi...@git.apache.org>.
Github user gorkem commented on the pull request:
https://github.com/apache/cordova-cli/pull/224#issuecomment-147553471
+1
---
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-cli pull request: CB-9784 Remove CLI logger levels prefixe...
Posted by nikhilkh <gi...@git.apache.org>.
Github user nikhilkh commented on a diff in the pull request:
https://github.com/apache/cordova-cli/pull/224#discussion_r41816178
--- Diff: src/logger.js ---
@@ -59,13 +58,12 @@ function formatError(error, isVerbose) {
logger.log = function (logLevel, message) {
if (this.levels[logLevel] >= this.levels[this.logLevel]) {
var isVerbose = this.logLevel === 'verbose';
- var prefix = this.prefixes[logLevel] ? this.prefixes[logLevel] + ': ' : '';
if(message instanceof Error) {
message = formatError(message, isVerbose);
}
- message = prefix + message + EOL;
+ message = message + EOL;
if (!this.cursor) {
this.output.write(message);
--- End diff --
I just realized that we should write error messages to stderr instead of output - is that correct?
---
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-cli pull request: CB-9784 Remove CLI logger levels prefixe...
Posted by shazron <gi...@git.apache.org>.
Github user shazron commented on the pull request:
https://github.com/apache/cordova-cli/pull/224#issuecomment-147543581
+1
---
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