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

Update to new interface of Good and es6 #67

Merged
merged 2 commits into from Mar 30, 2016
Merged

Update to new interface of Good and es6 #67

merged 2 commits into from Mar 30, 2016

Conversation

@thebergamo
Copy link
Contributor

thebergamo commented Mar 18, 2016

Well, this is a refactoring inspired for the issues #63 #64 and #65.

@arb can review?

NOTE: Well, I started this refactoring before the PR #66 are pushed, I have some delay to understand the new interface of Good and deal with Stream was a good xp.

@thebergamo thebergamo force-pushed the thebergamo:updates branch from b295d2e to 6fa0128 Mar 18, 2016
@arb arb self-assigned this Mar 18, 2016
@arb arb added this to the 6.0.0 milestone Mar 18, 2016
.travis.yml Outdated
@@ -2,5 +2,5 @@ sudo: false
language: node_js

node_js:
- 0.10
- 4
- 4

This comment has been minimized.

Copy link
@arb

arb Mar 18, 2016

Contributor

Need to be "4" and "node"

README.md Outdated
## Good Console Methods
### `goodconsole.init(stream, emitter, callback)`
Initializes the reporter with the following arguments:
## Usage

This comment has been minimized.

Copy link
@arb

arb Mar 18, 2016

Contributor

So this is just a standard transform stream. It's not tied to good at all so I don't think we need this usage here.

This comment has been minimized.

Copy link
@arb

arb Mar 18, 2016

Contributor

You should also mention that this stream is always in objectMode.

this._settings = Hoek.applyToDefaults(internals.defaults, config);
this._filter = new Squeeze(events);
};
class GoodConsole extends Stream {

This comment has been minimized.

Copy link
@arb

arb Mar 18, 2016

Contributor

Needs to be Stream.Transform

This comment has been minimized.

Copy link
@thebergamo

thebergamo Mar 18, 2016

Author Contributor

In this case I use const Stream = require('stream').Transform; above.

This comment has been minimized.

Copy link
@arb

arb Mar 18, 2016

Contributor

Opps.

This comment has been minimized.

Copy link
@arb

arb Mar 18, 2016

Contributor

I'd prefer to see Streams.Transform here so it's very clear what kind of stream this is.

@thebergamo thebergamo force-pushed the thebergamo:updates branch from 6fa0128 to 65817df Mar 18, 2016

/*eslint-disable */

This comment has been minimized.

Copy link
@arb

arb Mar 18, 2016

Contributor

What happened to this logic? We need this still somewhere.

This comment has been minimized.

Copy link
@thebergamo

thebergamo Mar 18, 2016

Author Contributor

tags isn't used on all events. Just in events: response (but is in event.logs[i].tags) log and request event.

This comment has been minimized.

Copy link
@arb

arb Mar 18, 2016

Contributor

If you look at the old code, I normalized this so that every event did include tags and the first tag was always the eventName.

} else if (data.tags != null) {
tags = [data.tags];
if (eventName === 'error') {
return next(null, this._formatError(data));

This comment has been minimized.

Copy link
@arb

arb Mar 18, 2016

Contributor

Rather than attaching this._formatError and this.formatOps and all the other formatters to this, just make them utility functions in this file since the consumer should never have a need to call them directly.

@thebergamo thebergamo force-pushed the thebergamo:updates branch from 65817df to 2965623 Mar 18, 2016

const Joi = require('joi');

exports.reporter = Joi.object().keys({

This comment has been minimized.

Copy link
@arb

arb Mar 18, 2016

Contributor

Using joi here is kind of overkill. Just use the default values logic that was there before.

@@ -9,22 +9,17 @@
"test-cov-html": "lab -r html -o coverage.html -La code"

This comment has been minimized.

Copy link
@arb

arb Mar 18, 2016

Contributor

Remove this while you're in there.

This comment has been minimized.

Copy link
@thebergamo

thebergamo Mar 18, 2016

Author Contributor

ok, coverage report will no more generated?

@@ -1,24 +1,29 @@
'use strict';

This comment has been minimized.

Copy link
@arb

arb Mar 18, 2016

Contributor

Please add the { plan: n } option to every test.


done();
});
it('throws an error if GoodConsole is called without "new"', (done) => {

This comment has been minimized.

Copy link
@arb

arb Mar 18, 2016

Contributor

Don't need this anymore.

var reporter = GoodConsole({ log: '*' });
expect(reporter._settings).to.exist();
const reporter = new GoodConsole();
expect(reporter).to.exist();

This comment has been minimized.

Copy link
@arb

arb Mar 18, 2016

Contributor

If you're going to keep this test, do an instanceof check.


StandIn.replace(process.stdout, 'write', function (stand, string, enc, callback) {
it('return a formatted string for "response" events', (done) => {

This comment has been minimized.

Copy link
@arb

arb Mar 18, 2016

Contributor

returns


event.timestamp = now;
const response = Hoek.clone(internals.response);

This comment has been minimized.

Copy link
@arb

arb Mar 18, 2016

Contributor

Rather than Hoek.clone, I think you can use Object.assign for the whole file.

const Stream = require('stream');

class Writer extends Stream.Writable {
constructor(objectMode) {

This comment has been minimized.

Copy link
@arb

arb Mar 18, 2016

Contributor

Since this is only used in objectMode, you don't need it as a parameter, just set it to true.


super({ objectMode });
this.data = [];
this.once('finish', () => {

This comment has been minimized.

Copy link
@arb

arb Mar 18, 2016

Contributor

You don't test this anywhere so it can probably be removed.

@thebergamo thebergamo force-pushed the thebergamo:updates branch from 2965623 to b3ef364 Mar 18, 2016
@thebergamo

This comment has been minimized.

Copy link
Contributor Author

thebergamo commented Mar 18, 2016

@arb can review?

README.md Outdated
@@ -10,33 +10,23 @@ Lead Maintainer: [Adam Bretz](https://github.com/arb)

`good-console` is a [good](https://github.com/hapijs/good) reporter implementation to write [hapi](http://hapijs.com/) server events to the console.

This comment has been minimized.

Copy link
@arb

arb Mar 19, 2016

Contributor

Change this to read something about a transform stream into formatted strings.

const reporter = new GoodConsole();
expect(reporter).to.exist();
expect(reporter).to.be.instanceof(GoodConsole);
expect(reporter.settings).to.exist();

This comment has been minimized.

Copy link
@arb

arb Mar 19, 2016

Contributor

Remove.

@thebergamo thebergamo force-pushed the thebergamo:updates branch from b3ef364 to 668cdc6 Mar 19, 2016
@thebergamo

This comment has been minimized.

Copy link
Contributor Author

thebergamo commented Mar 19, 2016

any other tips?

if (typeof event.responsePayload === 'object' && event.responsePayload) {
responsePayload = 'response payload: ' + SafeStringify(event.responsePayload);
config = config || {};
this.settings = Hoek.applyToDefaults(internals.defaults, config);

This comment has been minimized.

Copy link
@arb

arb Mar 21, 2016

Contributor

this._settings since we want to keep people out.


internals.response.timestamp = now;
describe('report', () => {

This comment has been minimized.

Copy link
@arb

arb Mar 21, 2016

Contributor

At the end of each test, call you push null through the read stream? Just to make sure good-console deals with nulls in the expected fashion.

@thebergamo thebergamo force-pushed the thebergamo:updates branch from 668cdc6 to d7e3e52 Mar 21, 2016
@thebergamo

This comment has been minimized.

Copy link
Contributor Author

thebergamo commented Mar 21, 2016

I update de README.md with the tags information on the example of outputs.

@thebergamo

This comment has been minimized.

Copy link
Contributor Author

thebergamo commented Mar 22, 2016

@arb some news?

@arb

This comment has been minimized.

Copy link
Contributor

arb commented Mar 22, 2016

@thebergamo I haven't forgotten; I will get to it when I can.

@thebergamo

This comment has been minimized.

Copy link
Contributor Author

thebergamo commented Mar 22, 2016

ok, sorry for my impatience :(

@arb arb merged commit a89a608 into hapijs:master Mar 30, 2016
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
2 participants
You can’t perform that action at this time.