Standard output stream by saggir · Pull Request #109 · SAP/vscode-logging

saggir · 2020-07-23T08:02:54Z

Adding support for console stream messages.
When enabling the Console options, messages that are printed using this library will be logged to the console stream (e.g. stdout/stderr in Linux or Debug Console in VSCode)

The Console stream is optional, default=false

You can enable it by setting: "logConsole: true" in the constructor options

This feature can be useful when debugging your extension.

CLAassistant · 2020-07-23T08:03:06Z

All committers have signed the CLA.

bd82 · 2020-07-23T08:11:19Z

 } = require("./settings");
 const { registerCommands } = require("./commands");
 const { listenToLogSettingsChanges } = require("./settings-changes-handler");
-const { callLibraryAndPassLogger } = require("./passing-logger-to-library");


don't remove part of this example

bd82 · 2020-07-23T08:13:23Z

  },
  "scripts": {},
  "dependencies": {
-    "@vscode-logging/library-example": "^0.1.2",


don't remove parts of this example

bd82 · 2020-07-23T08:17:55Z

    logOutputChannel: logOutputChannel, // OutputChannel for the logger
-    sourceLocationTracking: false
+    sourceLocationTracking: false,
+    standardOutput: false //define if messages should be logged to the standard output stream


missing whitespace between // and define

be consistent with previous property name:

logOutputChannel --> logStandardOut | logStdout

bd82 · 2020-07-23T08:18:41Z

+   * Warn, Info, Debug and Trace are printed to the standard output console
+   * Default = false
+   */
+  standardOutput?: boolean;


See comment above regarding the property name

Reading this comment it makes me thing the property name should maybe be
logConsole or logToConsole

Because your description uses the word console multiple times.
and console.error does not go to stdoutput stream

bd82 · 2020-07-23T08:21:01Z

@@ -1,5 +1,5 @@
 const { has } = require("lodash");
-const { createLogger } = require("winston");
+const winston = require("winston");


why was this imported changed?

bd82 · 2020-07-23T08:22:44Z

   */
  const transports = [];

+  if (opts.standardOutput) {


See the check above for "Logger must have at least one logging target defined, either logOutputChannel or logPath." you should update that as well

bd82 · 2020-07-23T08:29:59Z

@@ -0,0 +1,95 @@
+const { VSCodeStub } = require("./stubs/vscode-stub");


why does this test need a vscode stub? can we disable the outputChannel?

bd82 · 2020-07-23T08:34:18Z

+  let getExtensionLogger;
+  beforeEach(function() {
+    vsCodeStub = new VSCodeStub();
+    const mainModuleStubbed = proxyquire("../lib/api.js", {


I think you won't need the proxyQuire either if you remove the vscodestub

bd82 · 2020-07-27T15:30:29Z

@@ -1,4 +1,4 @@
-# @vscode-logging Extension Example
+    # @vscode-logging Extension Example


adding whitespace here messes up the file rendering

bd82 · 2020-07-27T15:49:03Z

+   * Warn, Info, Debug and Trace are printed to the standard output console
+   * Default = false
+   */
+  logStandardOutput?: boolean;


Naming and description of the property in this public API could be improved.

Property is called logStandardOutput which is a single stream's name

https://www.howtogeek.com/435903/what-are-stdin-stdout-and-stderr-on-linux/

But the description mentions it logs to either stdOut or stdErr.

Also the description refers to the term console, which may be an implementation detail.

Perhaps just expose the implementation details and call it logConsole or consoleLog?

bd82 · 2020-07-27T16:03:58Z

+    it("should not log to standard output when logStandardOutput is false", function() {
+      const extLogger = getExtensionLogger({
+        extName: "MyExtName",
+        logOutputChannel: vsCodeStub.OutputChannel,


Using a VSCode stub may be more complicated then using a File logger in this case. (logPath)

bd82

Some earlier comments were not addressed.
The main new comment is about the name of the new API.

I will not have time to review it again, proceed as you believe is best.

bd82 · 2020-07-29T08:35:51Z

You did not follow the contribution guidelines.

https://github.com/SAP/vscode-logging/blob/master/CONTRIBUTING.md#pull-request-guidelines

The commit message format is not by the convention, This means that the automatic changelog generation would
not work correctly.

bd82 · 2020-07-29T08:38:28Z

How were you able to bypass the commit message linter?

https://github.com/SAP/vscode-logging/blob/master/package.json#L39

Did you do git commit --no-verify?

bd82 · 2020-07-29T08:41:57Z

    logOutputChannel: logOutputChannel,
-    sourceLocationTracking: sourceLocationTrackingSettings
+    sourceLocationTracking: sourceLocationTrackingSettings,
+    logConsol: true


logConsol is a typo missing e (console)

bd82 · 2020-07-29T08:42:21Z

    logOutputChannel: logOutputChannel, // OutputChannel for the logger
-    sourceLocationTracking: false
+    sourceLocationTracking: false,
+    logConsol: false // define if messages should be logged to the consol


logConsol is a typo missing e (console)

bd82 · 2020-07-29T08:43:20Z

+   * Warn, Info, Debug and Trace are printed to the standard output console
+   * Default = false
+   */
+  logConsol?: boolean;


logConsol is a typo missing e (console)

bd82 · 2020-07-29T08:43:32Z

+  ) {
    throw Error(
-      "Logger must have at least one logging target defined, either logOutputChannel or logPath."
+      "Logger must have at least one logging target defined, it should includes one (or more) of these options: logOutputChannel, logPath and logConsol"


logConsol is a typo missing e (console)

bd82 · 2020-07-29T08:43:44Z

   */
  const transports = [];

+  if (opts.logConsol) {


logConsol is a typo missing e (console)

bd82 · 2020-07-29T08:44:11Z

    outChannel: opts.logOutputChannel,
-    sourceLocationTracking: opts.sourceLocationTracking
+    sourceLocationTracking: opts.sourceLocationTracking,
+    consolOutput: opts.logConsol


consoleOutput

bd82 · 2020-07-29T08:45:43Z

There is a general typo consol --> console
I marked some of the places but there are more.

Resolves Dependabot alerts via pnpm overrides for transitive dependencies. Packages updated: axios → 1.15.0 (#170) handlebars → 4.7.9 (#163, #162, #159, #158, #157, #156, #155, #153) tar → 7.5.13 (#147, #146, #128, #124, #120, #118) brace-expansion@2.x → 2.0.3 (#161, #154) js-yaml → 4.1.1 (#111) micromatch → 4.0.8 (#91) nanoid → 5.1.7 (#96) yaml → 2.8.3 (#150) diff → 5.2.2 (#119) tmp → 0.2.5 (#109)

bd82 self-requested a review July 23, 2020 08:10