Use theming-log for logging. by sttk · Pull Request #161 · gulpjs/gulp-cli

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

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


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

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

+    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

-  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

-  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

-  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

+  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

+    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

    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

-      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

+  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

@@ -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

+        log.on(level, consoleLog);
      }
+
+      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 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

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

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));

Uh oh!

Uh oh!

Conversation

sttk commented Apr 16, 2018

Uh oh!

sttk commented Jun 10, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sttk Jun 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sttk commented Jun 12, 2019

Uh oh!

sttk commented Jun 12, 2019

Uh oh!

phated commented May 20, 2020

Uh oh!

sttk commented May 20, 2020

Uh oh!

sttk commented May 20, 2020

Uh oh!

phated commented May 20, 2020

Uh oh!

phated commented May 20, 2020

Uh oh!

sttk commented May 20, 2020

Uh oh!

sttk commented May 21, 2020

Uh oh!

phated commented May 21, 2020

Uh oh!

phated commented Jan 7, 2024

Uh oh!

phated commented Feb 7, 2024

Uh oh!

sttk commented Feb 8, 2024

Uh oh!

phated commented Feb 9, 2024

Uh oh!

sttk commented Feb 12, 2024

Uh oh!

sttk commented Feb 12, 2024

Uh oh!

sttk commented Feb 12, 2024

Uh oh!

phated left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sttk Jun 10, 2019 •

edited

Loading

sttk Feb 17, 2024 •

edited

Loading