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

CommandHandler faster check #1074

Merged
merged 5 commits into from
Apr 17, 2018
Merged

CommandHandler faster check #1074

merged 5 commits into from
Apr 17, 2018

Conversation

wagnerluis1982
Copy link
Contributor

@wagnerluis1982 wagnerluis1982 commented Apr 13, 2018

Make the filters applied to a command to execute only if the command name matches. This solves the problem described in #1073

@wagnerluis1982
Copy link
Contributor Author

I forgot to say, this fixes #1073

Copy link

@erickmendonca erickmendonca left a comment

Choose a reason for hiding this comment

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

Looks good to me. Avoids processing filters if not needed.

@jh0ker
Copy link
Member

jh0ker commented Apr 14, 2018

Thanks a lot for your contribution @wagnerluis1982

The fix looks good, but it would be great to have a test to ensure we won't have regressions on this in the future.

I see you are a first-time contributor, would you like to add your name to the contributor list?

@jh0ker
Copy link
Member

jh0ker commented Apr 14, 2018

@wagnerluis1982 A good test case would be to show that a filter on a command handler is not executed when the command in a message does not match the command defined in the handler.

Copy link
Member

@tsnoam tsnoam left a comment

Choose a reason for hiding this comment

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

LGTM. Just waiting for tests to succeed (restarted the py3.5 on travis)

@tsnoam
Copy link
Member

tsnoam commented Apr 16, 2018

darn, forgot there are no unitests. yeah we need them.
@wagnerluis1982 if you don't manage to get to it we'll add them but it'll take us a while since we have lot on our plate.

@jh0ker
Copy link
Member

jh0ker commented Apr 16, 2018

I added a unit test 👍

@wagnerluis1982
Copy link
Contributor Author

@jh0ker Sorry the delay, I was off in the weekend.

The fix looks good, but it would be great to have a test to ensure we won't have regressions on this in the future.

I was so eager to fix the issue that I didn't remember tests 🙄. I see you already did, so I'm too late now...

I see you are a first-time contributor, would you like to add your name to the contributor list?

It's always good 😄. Since I actively develop one bot, I intend to contribute more to the project.

@tsnoam tsnoam merged commit 5efd5e2 into python-telegram-bot:master Apr 17, 2018
@tsnoam
Copy link
Member

tsnoam commented Apr 17, 2018

@wagnerluis1982 thanks for your contribution

@github-actions github-actions bot locked and limited conversation to collaborators Aug 21, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants