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.

Sign up for GitHub

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

Jump to bottom

Bugfix related to events which were excluded that had not tags #19

Merged
merged 1 commit into from
Nov 17, 2016
Merged

Bugfix related to events which were excluded that had not tags #19

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/squeeze.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ class Squeeze extends Stream.Transform {
}

// If include & exclude is an empty array, we do not want to do any filtering
if (subEventTags.include.length === 0 && subEventTags.exclude.length === 0) {
if (subEventTags.include.length === 0 && (subEventTags.exclude.length === 0 || tags.length === 0)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Couldn't we move the check for tags.length === 0 up to it's own line? Like if an even doesn't have any tags at all we can neither include or exclude them right?

Copy link
Contributor Author

@benleen benleen Nov 14, 2016 •
edited

Choose a reason for hiding this comment

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

This does mean that when you set up rules to only include events with tag "x", you will still also get the events without tags. Is this the desired behaviour?

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean move the check for tags.length === 0 and return false there and short circuit the rest of this logic? Looking at the tests, it looks like if the event doesn't have any tags, it returns false anyway. Then you wouldn't need this change I don't think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does return true in case we do include: '*' (which is the same as not defining include tags)
https://github.com/hapijs/good-squeeze/blob/master/test/index.js#L93-L98

As mentioned in the issue #18. When you add any exclude tags to the above all events without tags are excluded suddenly. Which is unwanted behaviour.

I might not get what you're trying to say all together. :-) Feel free to paste a bigger code snippet if you like in that case.

return true;
}

Expand Down
8 changes: 8 additions & 0 deletions test/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,14 @@ describe('Squeeze', () => {

done();
});

it('returns true if this reporter should report this event with only exclude tags defined in case no tags are present', { plan: 1 }, (done) => {

const subscription = Squeeze.subscription({ log: { exclude: 'debug' } });
expect(Squeeze.filter(subscription, { event: 'log' })).to.be.true();

done();
});
});

it('does not forward events if "filter()" is false', { plan: 1 }, (done) => {
Expand Down