Skip to content
This repository has been archived by the owner on Feb 1, 2019. It is now read-only.

rewrite for good v7.x.x #15

Merged
merged 1 commit into from
Jun 24, 2016
Merged

rewrite for good v7.x.x #15

merged 1 commit into from
Jun 24, 2016

Conversation

cjihrig
Copy link
Collaborator

@cjihrig cjihrig commented Jun 24, 2016

There is still one line of coverage needed. And I'm not sure what the good policy is now for filtering out the ops events that this module rejected in previous versions. But, I wanted to get eyes on it.

Closes #14

@arb
Copy link
Contributor

arb commented Jun 24, 2016

This looks fine to me. Do we know the reason "ops" events were filtered out? Did loggly not like them for some reason or did this person just not want them?

If it's the former, we can just check the event type and just eat it and not do anything about it. If it's the former, then users should use a transform stream before this to filter those out.

@arb arb modified the milestone: 3.0.0 Jun 24, 2016
@cjihrig
Copy link
Collaborator Author

cjihrig commented Jun 24, 2016

I'm not sure that it's actually a big deal - #11.

@arb
Copy link
Contributor

arb commented Jun 24, 2016

Then you can probably remove that stuff entirely. If you don't ops going into loggly, then you'd filter it out somewhere else.

@cjihrig
Copy link
Collaborator Author

cjihrig commented Jun 24, 2016

OK. Then I'm going to get coverage on the one missing line, and this should be good to go.

}
_write (event, encoding, callback) {
const data = Merge({}, event);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't THINK you need to do this anymore. Each reporter pipeline get's it's own copy so you are free to edit it as you see fit.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed

@cjihrig cjihrig merged commit 7a3c5fc into continuationlabs:master Jun 24, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants