Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Human reporter as Bunyan stream #36

Merged
merged 8 commits into from Jun 27, 2017
Merged

Human reporter as Bunyan stream #36

merged 8 commits into from Jun 27, 2017

Conversation

@artmnv
Copy link
Contributor

@artmnv artmnv commented Jun 27, 2017

Issue #34

@ai
Copy link
Member

@ai ai commented Jun 27, 2017

Looks great! Few notes:

  1. PID was lost from Human format :). Maybe we should add PID to Bunyan format to reuse it back in Human?
  2. You have ES6 classes to write BunyanFormatWritable class :).
  3. reporter() function in server.js is called on every message. Maybe it is not good idea to create Bunyan instance on every call? We can create one instance before defining reporter() as we did for reporter === "bunyan"
  4. If you use memory-stream only in test you should add it to devDependencies.
  5. done and expect.assertions(1) are not best way to do async tests. Why you can’t return Promise from test?
@ai
Copy link
Member

@ai ai commented Jun 27, 2017

BTW, you don’t need @name and @return JSDocs for ES6 classes.

@artmnv
Copy link
Contributor Author

@artmnv artmnv commented Jun 27, 2017

Thank you for your review!

  1. The issue with PID is that we have it in Bunyan log already. But it is in every log entry. We don't want to see pid in all entries as a param. So I don't know how to differentiate them. We can duplicate PID in something like details.pidFoo (we can't use simple details.pid). But I think it's ugly. Maybe we should better use something like
    ERROR [pid] Some mistake at 1970-01-01 00:00:00
    2 - 4. Done.
  2. Could you please provide an example of returning Promise from test? I made it like in jest docs: https://facebook.github.io/jest/docs/tutorial-async.html . BTW .resolves. doesn't work somehow.
@ai
Copy link
Member

@ai ai commented Jun 27, 2017

  1. In this case don’t need to send PID. I asked about PID because according snapshot changes in PR you lost PID from listen message in Human format. But PID is very important for logs.
@ai
Copy link
Member

@ai ai commented Jun 27, 2017

Async tests in Jest looks like:

it('works async', () => {
  return new asyncFunc().then(result => {
    expect(result.code).toEqual('good')
    return result.nextAsync()
  }).then(result => {
    expect(result.code).toEqual('good-again')
  })
})
@artmnv
Copy link
Contributor Author

@artmnv artmnv commented Jun 27, 2017

Look. I can't show pid in some of entries. Only in all of them. Like this:

ERROR  Missed client subprotocol requirements at 1970-01-01 00:00:00
             Check server constructor and Logux Server documentation
             Pid: 74689

But I think it's not what you've wanted. So I suggest you to show pid in all entries but like this:
ERROR [PID] Some mistake at 1970-01-01 00:00:00
What do you think?

@ai
Copy link
Member

@ai ai commented Jun 27, 2017

Why you can’t do it? You can message type and if it is listen, you can show PID.

@ai
Copy link
Member

@ai ai commented Jun 27, 2017

Why to you change:

-authed.user = { }
+authed.user = {}

If you want to change projects code style, you should send PR to eslint-config-logux first :).

@artmnv
Copy link
Contributor Author

@artmnv artmnv commented Jun 27, 2017

I don't like this kind of cohesion. But if you find it ok for this project..

@artmnv
Copy link
Contributor Author

@artmnv artmnv commented Jun 27, 2017

It's just prettier things ;) I didn't notice this change. Sorry. Eslint is OK with it.

@ai
Copy link
Member

@ai ai commented Jun 27, 2017

Never use prettier in PRs ;). If you wnt it, send it as separated PR.

@ai
Copy link
Member

@ai ai commented Jun 27, 2017

Not sure about this part:

+    delete rec.v
+    delete rec.name
+    delete rec.component
+    delete rec.hostname
+    delete rec.time

Changing origin object in formatter doesn’t seems like a good idea. Maybe it is better just ignore options from backlist?

But I don’t have good case for it. If you have any reasons we could keep it.

@artmnv
Copy link
Contributor Author

@artmnv artmnv commented Jun 27, 2017

Should I commit changing it back or squash commits and resend PR?

-authed.user = { }
+authed.user = {}
@ai
Copy link
Member

@ai ai commented Jun 27, 2017

I will squash whole PR, just add other commit on top

@ai
Copy link
Member

@ai ai commented Jun 27, 2017

Awesome. Thanks for all fixes. I will accept it after lunch (I will mark task as solved on Cult right now)

@ai
Copy link
Member

@ai ai commented Jun 27, 2017

Oops, seems like Travis CI is broken.

@artmnv
Copy link
Contributor Author

@artmnv artmnv commented Jun 27, 2017

Array.prototype.includes() polyfill is required. One moment.

@artmnv
Copy link
Contributor Author

@artmnv artmnv commented Jun 27, 2017

Thank you for your review and advices. :)

@ai ai merged commit 0e0bbbf into logux:master Jun 27, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.