Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update Filters, CommandHandler and MessageHandler #1221

Merged
merged 34 commits into from Feb 13, 2019

Conversation

@Eldinnie
Copy link
Member

Eldinnie commented Sep 21, 2018

See #1117 for discussion

Eldinnie added 7 commits May 22, 2018
Makes it possible to have filters work on an update instead of message, while keeping behavior for current filters
- remove allow_edited (deprecated for a while)
- set deprecated defaults to None
- Raise deprecation warning when they're used
- add sensible defaults for filters.
- rework tests
- rename update_types -> updates
- added some clarification to docstrings
@Eldinnie Eldinnie changed the base branch from master to V12 Sep 21, 2018
Copy link
Member

jsmnbom left a comment

While I do think that this change is overall good, it does introduce quite a lot of complexity to the user that wasn't there before (kwargs are easier to understand than negating filters imo). Therefore we need to be really sure that our docs are good.
That said, I think it mostly looks good. Mostly nitpicks :)

telegram/ext/commandhandler.py Show resolved Hide resolved
telegram/ext/commandhandler.py Outdated Show resolved Hide resolved
telegram/ext/filters.py Outdated Show resolved Hide resolved
telegram/ext/messagehandler.py Show resolved Hide resolved
telegram/ext/commandhandler.py Outdated Show resolved Hide resolved
tests/test_bot.py Show resolved Hide resolved
telegram/ext/commandhandler.py Outdated Show resolved Hide resolved
Eldinnie added 8 commits May 22, 2018
Makes it possible to have filters work on an update instead of message, while keeping behavior for current filters
- remove allow_edited (deprecated for a while)
- set deprecated defaults to None
- Raise deprecation warning when they're used
- add sensible defaults for filters.
- rework tests
- rename update_types -> updates
- added some clarification to docstrings
@jsmnbom jsmnbom modified the milestones: V11, V12 Sep 25, 2018
@jsmnbom jsmnbom mentioned this pull request Sep 25, 2018
Eldinnie added 3 commits Oct 1, 2018
@Eldinnie

This comment has been minimized.

Copy link
Member Author

Eldinnie commented Oct 1, 2018

Can't figure out how to get it to build on CI. Guess we will see when we merge onto v12 and then PR v12 onto pmaster

@jsmnbom

This comment has been minimized.

Copy link
Member

jsmnbom commented Oct 1, 2018

Might wanna remove the v12 from travis again. Though I wonder if the reason its not working is because of case sensitivity?
Or maybe the travis.yml file with "v12" needs to actually be on the v12 branch? (in which case just keep it like it is - maybe even change it so that it builds master and /^[vV]\d+$/

@Eldinnie Eldinnie mentioned this pull request Oct 1, 2018
Eldinnie added 2 commits Oct 4, 2018
@Eldinnie

This comment has been minimized.

Copy link
Member Author

Eldinnie commented Oct 4, 2018

Ok,

The last thing we need to decide IMO is what to do with context.match and Filters.regex
A couple of things have passed as an option:

  • Allow only one Filters.regex and out it's result in context.match
    • To decide. What to do when multiple regex filters are added?
  • Change context.match -> context.matches which will be a list.
    • will contain all the match object for regex filters (in sequential order)
    • means we will have to change regexhandler to also add it as a list
    • Will allow for phasing out RegexHandler and thus reducing the numbers of handlers (RegexHandler can then be substituted with MessageHandler(Filters.regex,. ..) for the same functionality

I prefer the second option.

@tsnoam

This comment has been minimized.

Copy link
Member

tsnoam commented Oct 4, 2018

@Eldinnie

I prefer the second option.

I agree with you here.

Also I suggest to keep RegexHandler as a wrapper over the new MessageHandler.
In general I think that for common patterns we should have wrappers as they'll be more user friendly.

@tsnoam

This comment has been minimized.

Copy link
Member

tsnoam commented Oct 4, 2018

after discussing the concept of the wrapper in the developers group I now understand that:

  1. The old RegexHandler will have less functionality than MessageHandler - it will be missing the filters argument.
  2. By adding the filters argument to RegexHandler it will be identical to MessageHandler

As of the above I change my mind on the subject of wrappers and suggest that we don't add filters argument toRegexHandler. Instead we should mark the entire handler as deprecated (and issue a warning).

@jsmnbom

This comment has been minimized.

Copy link
Member

jsmnbom commented Oct 4, 2018

I agree with that 👍

@Eldinnie Eldinnie requested a review from tsnoam Oct 8, 2018
telegram/ext/filters.py Outdated Show resolved Hide resolved
telegram/ext/filters.py Outdated Show resolved Hide resolved
telegram/ext/filters.py Show resolved Hide resolved
telegram/ext/messagehandler.py Outdated Show resolved Hide resolved
@jsmnbom jsmnbom mentioned this pull request Feb 8, 2019
jsmnbom added 4 commits Feb 8, 2019
- use data_filter instead of regex_filter on BaseFilter
- have these filters return a dict that is then updated onto CallbackContext instead of using a list is before
- Add a .match property on CallbackContext that returns .matches[0] or None
@jsmnbom jsmnbom dismissed their stale review Feb 8, 2019

Too many changes since review

@jsmnbom

This comment has been minimized.

Copy link
Member

jsmnbom commented Feb 13, 2019

Gave another lookover, merging with the intention of releasing a V12 beta to squash bugs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.