Allow filters to have a name. #675
Conversation
So their string representation is user friendly.
So just remove it for now. Better than doing annoying stuff with parsing a **kwargs dict in my opinion. It didn't even *really* need to be kwarg only anyways I guess...
Increases coverage by 1 (one) line! Woo :D
tsnoam
left a comment
tsnoam
left a comment
There was a problem hiding this comment.
looks good in general. comments on code with some ideas on how to make the code more fluent.
telegram/ext/filters.py
Outdated
| self.name = self.__class__.__name__ | ||
| return self.name | ||
|
|
||
| __repr__ = __str__ |
There was a problem hiding this comment.
it's sufficient to implement __repr__. It will implicitly get __str__ working as you want
telegram/ext/filters.py
Outdated
| return "<telegram.ext.filters.InvertedFilter inverting {}>".format(self.f) | ||
| return "<inverted {}>".format(self.f) | ||
|
|
||
| __repr__ = __str__ |
There was a problem hiding this comment.
same comment about __repr__ vs __str__
telegram/ext/filters.py
Outdated
|
|
||
| def __str__(self): | ||
| # Do not rely on classes overwriting __init__ to set a name | ||
| # so we can keep backwards compatibility |
There was a problem hiding this comment.
what about defining name as class attribute which defaults to '<user_filter>' (or something of the sort)?
then we can save the ugly code below...
Following that idea, you can then add name attribute to each of the inheriting classes which will reduce the need to call super().__init__() and modify all the lines instantiating the filters.
There was a problem hiding this comment.
on a second thought: what's wrong with using self.__class__.__name__?
There was a problem hiding this comment.
Currently it's usingself.__class__.__name__ only if no other name is found. The reason we don't use that is that telegram.ext.Filters.status_update.migrate.__class__.__name__ is _Migrate and not migrate as we want. So we have to overwrite it.
Or did I misunderstand your question?
I would prefer if it did use self.__class__.__name__ when a better name is not found though. So if we had name as a class attribute and defaulted it to None (where it would then do self.__class__.__name__) would that work?
There was a problem hiding this comment.
so back to my first suggest. name = <user_filter> by default
There was a problem hiding this comment.
But I'd rather have it be self.__class__.__name__ by default though...?
|
|
||
| def __init__(self, name=None): | ||
| self.name = name | ||
|
|
There was a problem hiding this comment.
This can be avoided, see my comment in __str__ below
telegram/ext/filters.py
Outdated
| return "<{} {} {}>".format(self.base_filter, "and" if self.and_filter else "or", | ||
| self.and_filter or self.or_filter) | ||
|
|
||
| __repr__ = __str__ |
There was a problem hiding this comment.
same comment about __repr__ vs __str__
So their string representation is user friendly.
Compare:
Old
New