Adds callback parameter to the `log` and other functions. by vilyapilya · Pull Request #73 · logdna/nodejs

vilyapilya · 2019-09-12T06:57:25Z

Adds an option to pass a callback when the log (or any other logging function) is called. It will pass the object with the response code and the response or an error to the callback after sending the request to the server.

dchai76

This is going to take me some time to review, I'm going to start with just one comment

dchai76 · 2019-10-15T19:25:30Z


    if (opts) {
-        if (typeof opts === 'string') {
+        if (typeof opts === 'function') {


We seem to do this a lot and it seems to me like a terrible idea - it's confusing to me that we can let people send in a callback function as the opts param. Wouldn't it be preferable to require them to send in an empty/null opts and be explicit about what callback is than this?

Didn't answer my question, but I really dislike this - let's keep each parameter as it's meant to be instead of pseudo-overloading.

sorry I did not see your comments.

@dchai76 I think it's arguable. On one hand the implementation of the code library becomes harder to read and maintain. On the other hand, usage of our code libraries becomes easier (you don't have to write your code likes this logger.log("my lolg", {}, (err) => {if err stdout)}); every time when you need to pass the cb to the log function. I think it's better to take the burden of readability on us than pass it on users.

and besides, we can also pass a level to that. And it can grow into log("logline", "", {}, cb). Which I think is not a really good practice.

To be clear, I think this is bad for callers, not for our own maintainability - magic overloading can mask errors on their part. Did they mean to forget to include opts? Or was it intentional? The type of magic where we say one parameter can be treated in different ways is surprising, and not in a good way.

I personally don't like this pattern, but I do see this used in lots of places :(
I guess the best way to escape is the make it an async function that returns a promise, so up to the caller whether to chain it or not

@weizou19 pointed out to me that Node itself does this (magic overloading of provided callbacks) (https://github.com/nodejs/node/blob/master/lib/fs.js#L405-L414) so as much as I dislike it I'm going to say I'm wrong on this.

dchai76 · 2019-10-17T01:07:55Z


    if (opts) {
-        if (typeof opts === 'string') {
+        if (typeof opts === 'function') {


Didn't answer my question, but I really dislike this - let's keep each parameter as it's meant to be instead of pseudo-overloading.

dchai76 · 2019-10-17T01:11:53Z

+            callback = opts;
+            opts = {};
+        }
+        if (!opts) opts = {};


I prefer opts = opts || {}; format. If you do have it this way, let's bracket the clause in the if - 1 line if statements like this can lead to errors when refactoring.

dchai76 · 2019-10-17T01:17:57Z

    let sentLevels = [];
+
+    let callbackResult;
+    const testCallback = (er, res) => { callbackResult = res;};


Correct me if I'm wrong, but it appears the only place the callback is actually called (and not just passed along) is in Logger._flush(), and that seems to call callback without any params, so how does this work?

@dchai76 it attaches the callback to the instance line 340 and in the async function send_request it calls the callback that was passed specifically for that instance.

dchai76 · 2019-10-24T16:11:48Z


    if (opts) {
-        if (typeof opts === 'string') {
+        if (typeof opts === 'function') {


@weizou19 pointed out to me that Node itself does this (magic overloading of provided callbacks) (https://github.com/nodejs/node/blob/master/lib/fs.js#L405-L414) so as much as I dislike it I'm going to say I'm wrong on this.

dchai76 · 2019-10-24T16:12:38Z

 configs.LOG_LEVELS.forEach(function(level) {
-    var l = level.toLowerCase();
-    Logger.prototype[l] = function(statement, opts) {
+    var levelFuncationName = level.toLowerCase();


Make this a const and fix the spelling

We should set "no-var": "error" to enforce it: https://github.com/logdna/nodejs/blob/master/.eslintrc#L219

Next PR for it

dchai76 · 2019-10-24T16:21:05Z

            done();
        }, configs.FLUSH_INTERVAL + 200);
    });
+    it('Debug Functio, when called with callback, should call the cb', function(done) {


While it's up to you, I would add a nested describe for these (describe('Debug Function'...) so that the actual test names only say the expected behavior (it('Executes callback when provided'...)`)

weizou19

LGTM

smusali

LGTM too since I can see more people may need to use this - @dchai76 @mikehu do you have any extra comment on this Pull Request?

dchai76 · 2019-10-24T22:14:19Z

    }

+    if (!callback || typeof callback !== 'function') {
+        callback = (err) => {debug(err);};


Would prefer spaces between the brackets

Another linting rule to add
https://github.com/logdna/nodejs/blob/master/.eslintrc#L226
"space-in-brackets": "always"

dchai76 · 2019-10-24T22:14:39Z

    let sentLevels = [];
+
+    let callbackResult;
+    const testCallback = (er, res) => { callbackResult = res;};


Would prefer a space before the closing bracket

lvilya added 2 commits September 11, 2019 23:52

add a callback to logging functionality

835c136

lints

023b188

smusali requested review from dchai76, respectus and smusali October 15, 2019 19:11

smusali self-assigned this Oct 15, 2019

smusali added the enhancement label Oct 15, 2019

dchai76 reviewed Oct 15, 2019

View reviewed changes

dchai76 suggested changes Oct 17, 2019

View reviewed changes

dchai76 requested a review from weizou19 October 23, 2019 00:29

address some comments

59255b4

dchai76 reviewed Oct 24, 2019

View reviewed changes

lvilya added 4 commits October 24, 2019 11:44

nested describe block.

523553e

lints

760d93c

spelling

669b426

bhvsdf

0e6327b

weizou19 approved these changes Oct 24, 2019

View reviewed changes

smusali requested review from dchai76, mikehu and weizou19 October 24, 2019 21:56

smusali approved these changes Oct 24, 2019

View reviewed changes

dchai76 approved these changes Oct 24, 2019

View reviewed changes

space between the brackets

13e64d9

vilyapilya merged commit c1b4fae into master Oct 24, 2019

smusali deleted the addCallbackToLog branch October 24, 2019 22:23

Conversation

vilyapilya commented Sep 12, 2019

Uh oh!

dchai76 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!

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!

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!

weizou19 left a comment

Choose a reason for hiding this comment

Uh oh!

smusali 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants