Merged
Conversation
Should ideally superseed #375.
We're testing for a string in list, not the other way around :P
5 tasks
jh0ker
requested changes
Sep 20, 2016
Member
jh0ker
left a comment
There was a problem hiding this comment.
I think you got a bit confused here.
The filters list of MessageHandler is also operating on an OR basis (at least up until v5.0). If I'm not mistaken, the example in the docstring does not do what it says it does.
In the light of #411, I don't think we should use a list here at all, it will be too confusing once the bitwise operators are in the game.
I recommend to change it to singular Filters.entity(entity_type). This also makes the docstring less of an issue 👍
Member
|
I just realized that I was the one to say we should use a list here... To be fair though, the #411 PR was not planned at that point. And I still like the higher-order function. |
Also remove the faulty example completely since it should be no longer needed.
jh0ker
approved these changes
Sep 24, 2016
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Should ideally superseed #375.
We could also add individual filters for each Entity (which use this function)...
Not entirely happy with the docstring at line 98-99, but I think it's the best we can do.
This is the first of two revisions to the filter system (the second one will change the way to AND/OR them).