Bugfix related to events which were excluded that had not tags #19
Conversation
| @@ -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)) { | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
How would you like me to document? It seemed like a bug to me. |
|
Thanks @arb! Sorry for not adding the label 'bug' right away! |
|
No worries, we were still debating it being a bug or not 😄 |
|
This thread has been automatically locked due to inactivity. Please open a new issue for related bugs or questions following the new issue template instructions. |
Resolves #18