Skip to content

Conversation

@pierreluctg
Copy link
Collaborator

For example when using can.logger the default can_filters value is a empty list, this leads to filtering all message. This change fix the issue by treating any empty Iterator same as filters=None

For example when using `can.logger` the default filter value is a empty list, this leads to filtering all message. This change fix the issue by treating any empty Iterator as filter==None
@pierreluctg
Copy link
CollaboratorAuthor

pierreluctg commented Jun 12, 2018

@hardbyte What is your opinion on that one?

Currently this PR is not passing the match_nothing test in https://github.com/hardbyte/python-can/blob/develop/test/test_message_filtering.py#L49 intruduced by @felixdivo in #277

I am not sure is this test make sense or not. When looking at the interfaces that supports filtering directly, they treat a empty Iterator as no filtering (opposite of the test)

@felixdivo
Copy link
Collaborator

When I implemented that API, I did not find any precise or central documentation on how to handle can_filters. I simply have treated empty iterables like I thought it to be the most intuitive. That's the story behind the change I made.

@felixdivo
Copy link
Collaborator

When this is actually changed, please update the documentation accordingly.

@pierreluctg
Copy link
CollaboratorAuthor

Tests and doc have been updated

hardbyte
hardbyte previously requested changes Jun 12, 2018
can/bus.py Outdated
All messages that match at least one filter are returned.
If `filters` is `None`, all messages are matched.
If it is a zero size interable, no messages are matched.
If `filters` is `None` or a a zero size interable, all
Copy link
Owner

Choose a reason for hiding this comment

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

typo here. I'd say "a zero length sequence"

@felixdivofelixdivo merged commit e6d9e76 into hardbyte:developJun 13, 2018
@pierreluctgpierreluctg deleted the patch-1 branch August 14, 2018 17:46
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@pierreluctg@felixdivo@hardbyte