Use theming-log for logging. #161

sttk · 2018-04-16T15:41:48Z

I've implemented configurable logs with theming-log.

But it is needed js-liftoff#95 to configure logs for require flag and completion flag.

sttk · 2019-06-10T12:01:23Z

I updated this pr based on the current master branch.

sttk · 2019-06-10T20:44:46Z

docs/html/js/write-msg-list.js

@@ -0,0 +1,117 @@
+'use strict';


Sorry. This file is not used. I'll remove it.

sttk · 2019-06-10T20:50:58Z

docs/html/js/theme-dic.js

+    params: [
+      { example: 'This is a code message.' },
+    ],
+  },


Sorry. I mistook about items from ansi-colors. There are more items and code does not exist. I'll modify them.

sttk · 2019-06-12T13:53:58Z

I've modified the above two points about document.

sttk · 2019-06-12T14:25:07Z

This is a sample:

$ cat .gulp.js
module.exports = {
  log: {
    theme: require('./my-theme'),
    messages: require('./my-msgs'),
  }
};
$ cat my-theme.json 
{
  "ICON": "🥤",
  "VERSION": "{bold: {magenta: {1}}}",
  "TIMESTAMP": "{yellow: [{NOW: YYYY/MM/DD HH:mm:ss}]} ",
  "DESC": "{cyan: {1}}"
}
$ cat my-msgs.json 
{
  "info": {
    "version": "{ICON} {DESC: Gulp version}: {VERSION: {2}}\n{ICON} {DESC: CLI version}: {VERSION: {1}}"
  },
  "error": {
    "gulpfileNotFound": "{ICON} {TIMESTAMP}{ERROR: No gulpfile found}"
  }
}

phated · 2020-05-20T06:01:18Z

@sttk I'd like to start work on this again. I noticed the HTML files in this PR - what are those used for?

sttk · 2020-05-20T11:58:56Z

@phated Thanks.
HTML files (and other files in docs/html) are sample documents to configure themes and messages. It's not necessary and use as refernece of Gulp document.

sttk · 2020-05-20T16:30:22Z

@phated I created a new branch in my repository, which was based on the current master (v2.2.1), was merged with the change of this PR except doc/html, and was modified for #177.

https://github.com/sttk/gulp-cli/tree/use_theming-log_2

Should I update this PR with that new branch?

phated · 2020-05-20T16:58:35Z

@sttk Yes, please update this branch. It needed a rebase on master anyway.

I hope to find time to review soon.

phated · 2020-05-20T16:59:00Z

@sttk would you want to include your gulp libraries inside the gulpjs organization?

sttk · 2020-05-20T23:32:14Z

@phated I've updated!

would you want to include your gulp libraries inside the gulpjs organization?

Yeah, it's good.
I'd like to include copy-props, each-props and theming-log into gulp organization.

sttk · 2020-05-21T02:14:12Z

I forgot to test about #177, and did it.

phated · 2020-05-21T03:03:10Z

@sttk thank you! I have confirmed that gulpjs members can create and transfer repositories, so you can transfer those projects when you have time.

phated · 2024-01-07T20:56:38Z

@sttk I believe this needs conflicts resolved since I merged #239

phated · 2024-02-07T00:03:54Z

@sttk are you planning to bring this up-to-date for the gulp-cli v3.0.0 release or should I plan to finish the remaining tasks without color configuration?

sttk · 2024-02-08T03:58:59Z

@phated I'm tackling this issue. Hopefully, I'll finish this weekend.
I hope to include this feature into gulp-cli v3.0.0. Please give me a little more time.

https://github.com/sttk/gulp-cli/commits/ci-test/

phated · 2024-02-09T05:50:16Z

@sttk 👍 sounds good. I will wait for your changes.

sttk · 2024-02-12T03:52:27Z

@phated I've completed the changes for this PR. Please review it.

sttk · 2024-02-12T08:40:07Z

I've added handling for yargs error message.

…cription.

…iption is added.

sttk · 2024-02-12T16:14:54Z

I've made several changes. In Addition, I've removed config.description as its purpose overlaps with config.log.msgs.tasks/tasksJson.description.

phated

Thanks for preparing this @sttk 🙏

I'm pretty concerned how much needs to change to support this feature. Is there a way we can reduce it?

phated · 2024-02-14T03:28:50Z

index.js

-  var pid = chalk.magenta(child.pid);
-  log.info('Node flags detected:', nodeFlags);
-  log.info('Respawned to PID:', pid);
+  log.info(msgs.info.respawn, flags.join(', '), child.pid);


These should not be combined, one message is about detecting node flags and one if about the respawn.

It seems that users configuring this would consider these two lines as one.
Additionally, in this way, it is also possible to either output only one line or not output both lines.

phated · 2024-02-14T03:29:33Z

index.js

-  if (error) {
-    log.warn(chalk.yellow(error.toString()));
-  }
+  log.warn(msgs.warn.preloadFailure, name, Boolean(error), error.toString());


I don't like combining the preload message failure and the error printing. Can we keep them separate?

phated · 2024-02-14T03:29:39Z

index.js

-  if (error) {
-    log.warn(chalk.yellow(error.toString()));
-  }
+  log.warn(msgs.warn.loaderFailure, name, Boolean(error), error.toString());


I don't like combining the preload message failure and the error printing. Can we keep them separate?

phated · 2024-02-14T03:30:04Z

index.js

+  if (optsErr) {
+    log.error(msgs.error.failToParseCliOpts, optsErr.message);
+    makeHelp(parser).showHelp(console.error);
+    exit(1);
+  }


Why do we need this?

This part is needed against an error process of yargs for invalid option.
For example, gulp -f causes a yargs error because -f/—gulpfile requires an argument. By default, yargs outputs the error message Not enough arguments following: f, outputs help messages, and exits the program.
To control this behavior and configure these messages, I added this part and another part in lib/shared/options/parser.js.

phated · 2024-02-14T03:30:45Z

index.js

+    var gulpVersion = env.modulePackage.version || 'Unknown';
+    console.log(format(theme, msgs.info.version, cliVersion, gulpVersion));


I don't think we want to allow this to be configured. We ask for this specific information in our bug reports.

Anywhere we use console.log instead of the logger we don't need to support configuration.

Understood. I think that's reasonable.

phated · 2024-02-14T03:41:26Z

lib/shared/log/tasks.js

  var maxLabelWidth = 0;

  tree.nodes.forEach(function(node, idx, arr) {
    var isLast = idx === arr.length - 1;
    var w = createTreeLines(node, lines, opts, 1, '', isLast);
-    maxLabelWidth = Math.max(maxLabelWidth, w || 0);
+    maxLabelWidth = Math.max(maxLabelWidth, w || /* istanbul ignore next */ 0);


Why add this comment now?

For increasing coverage. However, in the first place,|| 0 is unnecessary.

phated · 2024-02-14T03:42:49Z

lib/shared/log/tasks.js

-      var spaces = ' '.repeat(maxLabelWidth - line.label.length) + '  ';
-      s += spaces + line.desc;
+      var spaces = ' '.repeat(maxLabelWidth - line.width) + '  ';
+      log.info(line.fmt, line.bars, line.name, true, spaces, line.desc);


Do you think users will configure this?

At least, those colors should be configurable according to console background color.

phated · 2024-02-14T03:44:12Z

lib/shared/log/theme.js

+  IF: function(text) {
+    var idx = text.indexOf('?');
+    var cond = text.substring(0, idx).trim();
+    if (cond === '' || cond === 'false') {
+      return '';
+    }
+    return text.slice(idx + 1);
+  },


Where do we use this? Can we avoid it?

phated · 2024-02-14T03:44:46Z

lib/shared/log/timestamp.js

@@ -0,0 +1,55 @@
+'use strict';
+
+function timestamp(format) {


What is this used for?

This function is used to display the timestamp at the beginning of logs, similar to fancy-log.
Some users may want to append dates to timestamps, while others may not want to display the timestamp itself for specific messages like #206. There may also be users who want to change the color, as in gulpjs/gulp#2271. To meet such demands, I made displaying timestamp configurable, too.

phated · 2024-02-14T03:46:25Z

lib/shared/log/to-console.js

      }
+
+      log[level] = themingLog(theme, log[level]);


I'm not sure why you are doing this but I don't think I want to override the logging functions here.

You're right! This is wrong. This code replaces global gulplog methods, affecting other plugins that use gulplog. Instead of gulplog, we should create our own logger specific to gulp-cli. To use own logger would also ~~eliminate~~ separate the need to consider the issue #249 (and #254).
One idea I have is to pass the namespace Symbol('gulp-cli') to glogg to create a new logger.

--
This is a digression and a future discussion, but could we potentially resolve #72 by creating loggers by glogg with namespace by task names and plugin names and establishing rules for plugins to use it? It’s still rough idea, and I don’t have thought of specific methods yet.

I've mistaken. Initialization of the logging functions of gulplog v1 is necessary, even so.

sttk · 2024-02-17T07:14:21Z

@phated I've modified what you pointed out except some points still under discussion.

sttk · 2024-03-06T12:04:50Z

@phated I want to add --no-theme cli flag and flags.noTheme config flag that are not to use log theme and messages in a config file. I believe they could be useful when reporting issues or when not using the theme specified in the project.

Can I add them to this PR?

phated mentioned this pull request Jan 2, 2019

Add theming gulpjs/fancy-log#19

Closed

sttk mentioned this pull request Jun 9, 2019

Change timing of completion to enable to configure its error message gulpjs/liftoff#106

Merged

sttk force-pushed the use_theming-log branch from cd9ceb2 to ba5bdc8 Compare June 10, 2019 10:51

sttk commented Jun 10, 2019

View reviewed changes

docs/html/js/write-msg-list.js Outdated

@@ -0,0 +1,117 @@

'use strict';

Copy link

Contributor Author

sttk Jun 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry. This file is not used. I'll remove it.

sttk commented Jun 10, 2019

View reviewed changes

sttk mentioned this pull request May 20, 2020

Log only the "Finished" messages and not the "Starting" ones #177

Closed

sttk force-pushed the use_theming-log branch from e9df7a9 to a858b41 Compare May 20, 2020 23:00

sttk force-pushed the use_theming-log branch from a858b41 to 1a3b82e Compare May 21, 2020 01:16

phated self-requested a review October 21, 2020 21:52

sttk added 2 commits February 12, 2024 12:13

feat: make log messages and styles configurable with theme.

be0955c

feat: enable to override log messages and theme with a config file.

9034164

sttk force-pushed the use_theming-log branch from 6265e8b to 9034164 Compare February 12, 2024 03:42

fix: add handling for yargs message when cli options are invalid.

2d3a80d

sttk added 3 commits February 12, 2024 23:36

fix: rename msgs.tasks/tasksJson.gulpfile to msgs.tasks/tasksJson.des…

a39ab8c

…cription.

fix: modify positions of msgs and theme in config

3a95fd3

update: remove config.description because config.log.msgs.tasks.descr…

741ff5a

…iption is added.

phated requested changes Feb 14, 2024

View reviewed changes

sttk added 4 commits February 17, 2024 11:02

fix: remove a comment ignoring coverage

a4bfdd5

fix: remove a comment for ignoring coverage and add a test case

237cb58

fix: add tests for overrideEnvConfig

ba976ab

fix: remove the version message from the configuration target

d8e01e8

phated mentioned this pull request Mar 23, 2024

feat: Support theming and translations via config files #260

Merged

phated closed this in #260 Mar 23, 2024

		var gulpVersion = env.modulePackage.version \|\| 'Unknown';
		console.log(format(theme, msgs.info.version, cliVersion, gulpVersion));

Use theming-log for logging. #161

Use theming-log for logging. #161

Conversation

sttk commented Apr 16, 2018

sttk commented Jun 10, 2019

Choose a reason for hiding this comment

sttk Jun 10, 2019 • edited Loading

Choose a reason for hiding this comment

sttk commented Jun 12, 2019

sttk commented Jun 12, 2019

phated commented May 20, 2020

sttk commented May 20, 2020

sttk commented May 20, 2020

phated commented May 20, 2020

phated commented May 20, 2020

sttk commented May 20, 2020

sttk commented May 21, 2020

phated commented May 21, 2020

phated commented Jan 7, 2024

phated commented Feb 7, 2024

sttk commented Feb 8, 2024

phated commented Feb 9, 2024

sttk commented Feb 12, 2024

sttk commented Feb 12, 2024

sttk commented Feb 12, 2024

phated left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sttk Feb 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sttk commented Feb 17, 2024

sttk commented Mar 6, 2024

sttk Jun 10, 2019 •

edited

Loading

sttk Feb 17, 2024 •

edited

Loading