Fix typos in Filters #3272
Fix typos in Filters #3272
Conversation
telegram/ext/filters.py
Outdated
| @@ -182,8 +183,16 @@ def __init__(self, name: str = None, data_filter: bool = False): | |||
| self._name = self.__class__.__name__ if name is None else name | |||
| self._data_filter = data_filter | |||
|
|
|||
| def check_update(self, update: Update) -> Optional[Union[bool, DataDict]]: # skipcq: PYL-R0201 | |||
| """Checks if the specified update is a message.""" | |||
| def check_update(self, update: Update) -> bool: # skipcq: PYL-R0201 | |||
There was a problem hiding this comment.
the type hint change should be reverted. The subclasses of BaseFilters check_update can return DataDict based on data_filter property. That's why pylint also complained in UpdateFilter and MessageFilter
There was a problem hiding this comment.
@harshil21 Are you sure? the base method can only return the True of False 🤔 The methods which overrides this one can return the DataDict 🤔
There was a problem hiding this comment.
imo, base method should have same sig as children (if possible). Let's see what others think
There was a problem hiding this comment.
@harshil21 I reverted the change for now, we can do it in another PR to get others opinions on this specific case later 👍
rglsk
force-pushed
the
3109-update-ext-filters
branch
from
85cf29b
to
cbdbd57
Compare
There was an issue that base method returned only bool but typing was saying something else just to match pylint override error.
…ntion. Most of the code bases there is convention to start docstring with a new line.
rglsk
force-pushed
the
3109-update-ext-filters
branch
from
cbdbd57
to
704b04f
Compare
| update (:class:`telegram.Update`): Incoming update. | ||
|
|
||
| Returns: | ||
| :obj:`bool` | Dict[str, list] |
There was a problem hiding this comment.
| :obj:`bool` | Dict[str, list] | |
| :obj:`bool` | Dict[:obj:`str`, :obj:`list`] |
| """ | ||
| Base class for all Filters. |
There was a problem hiding this comment.
TBH I don't really see any value in these changes - in this PR or even at all, that is.
True, some style guides suggest to start the docstring on a new line, but none of the styling tools that we currently use will report on these things. If we want to have docstrings start on a new line, I would rather like to make it a standalone issue to apply this to the whole code base.
I'm inclined to ask to revert these changes. Let's first hear what others think, though.
There was a problem hiding this comment.
I also agree to revert, they are rendered the same and PEP 257 does not comment on this design.
|
Alright folks, sorry for that, I'll try better next time 🙏 |
Refer to #3109, done:
check_updatemethod (@Bibo-Joshi : Please let me know if this change should be cherry-picked to other branch to merge across master -> I'm happy to do if if the proposed suggestion fits you 😄 )Checklist for PRs
.. versionadded:: version,.. versionchanged:: versionor.. deprecated:: versionto the docstrings for user facing changes (for methods/class descriptions, arguments and attributes)AUTHORS.rst(optional)__all__s