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 - ignore strings in entities and "/" followed by whitespace #1020

Merged
merged 4 commits into from Mar 1, 2018

Conversation

Projects
None yet
3 participants
@PaulSonOfLars
Contributor

PaulSonOfLars commented Feb 24, 2018

Telegram entities show that / test is not a valid command. However,
PTB accepts this as a valid command due to how the message splitting is
done.

This commit makes sure that any commands are indeed in the format of
/test, with no space between / and test.

PaulSonOfLars and others added some commits Feb 24, 2018

Ensure only valid commands are accepted.
Telegram entities show that `/ test` is not a valid command. However,
PTB accepts this as a valid command due to how the message splitting is
done.

This commit makes sure that any commands are indeed in the format of
`/test`, with no space between `/` and `test`.
@tsnoam

This comment has been minimized.

Show comment
Hide comment
@tsnoam

tsnoam Feb 25, 2018

Member

@PaulSonOfLars
Thanks for your contribution. It is a great observation that leading spaces should be properly handled, as well as ignoring commands if they are part of a markup entity.

I pushed a really small fix for the PR and making sure that unitest pass.
I'm not sure why the unitests on Windows failed before, but if they'll pass this time I'll merge, otherwise I'll look into the failure more thoroughly.

Member

tsnoam commented Feb 25, 2018

@PaulSonOfLars
Thanks for your contribution. It is a great observation that leading spaces should be properly handled, as well as ignoring commands if they are part of a markup entity.

I pushed a really small fix for the PR and making sure that unitest pass.
I'm not sure why the unitests on Windows failed before, but if they'll pass this time I'll merge, otherwise I'll look into the failure more thoroughly.

@Eldinnie

This comment has been minimized.

Show comment
Hide comment
@Eldinnie

Eldinnie Feb 25, 2018

Member

Small question, why do you check on message.text_markdown instead of message.text?

Member

Eldinnie commented Feb 25, 2018

Small question, why do you check on message.text_markdown instead of message.text?

@tsnoam

This comment has been minimized.

Show comment
Hide comment
@tsnoam

tsnoam Feb 25, 2018

Member

@Eldinnie That's a smart change. Suppose the message sent to the bot is __/start__ - this is not a command...

Member

tsnoam commented Feb 25, 2018

@Eldinnie That's a smart change. Suppose the message sent to the bot is __/start__ - this is not a command...

@Eldinnie

This comment has been minimized.

Show comment
Hide comment
@Eldinnie

Eldinnie Feb 25, 2018

Member

@tsnoam Ah that is indeed true. However /吴语 isn't a command either, and we do accept those as commands. Changing the behavior breaks with what people axpect and this changes more then only the proposed change.

Member

Eldinnie commented Feb 25, 2018

@tsnoam Ah that is indeed true. However /吴语 isn't a command either, and we do accept those as commands. Changing the behavior breaks with what people axpect and this changes more then only the proposed change.

@tsnoam

This comment has been minimized.

Show comment
Hide comment
@tsnoam

tsnoam Feb 25, 2018

Member

@Eldinnie We'll still support "/吴语" (up to potential problems with py2)
however, a single "/" with a space afterwards is definitely not a command, i really don't think we should support that.

Member

tsnoam commented Feb 25, 2018

@Eldinnie We'll still support "/吴语" (up to potential problems with py2)
however, a single "/" with a space afterwards is definitely not a command, i really don't think we should support that.

@tsnoam tsnoam changed the title from Ensure only valid commands are accepted. to CommandHandler - ignore strings in entities and "/" followed by whitespace Feb 25, 2018

@tsnoam

This comment has been minimized.

Show comment
Hide comment
@tsnoam

tsnoam Feb 25, 2018

Member

Fixed the PR title to be a bit more accurate.

Member

tsnoam commented Feb 25, 2018

Fixed the PR title to be a bit more accurate.

@Eldinnie

This comment has been minimized.

Show comment
Hide comment
@Eldinnie

Eldinnie Feb 25, 2018

Member

Ok so I found out what the trouble was. (And I;m pretty surprised it didn't give any trouble on other platforms).
The test bot we use has the username ptb_test_bot which was only escaped properly on AppVeyor to ptb\_text\_bot Why this only happened on appveyor is a mystery for me. But it seems using text_html instead of text_markdown fixes the problem for this PR.

If CI finishes properly now it;s good to merge

Member

Eldinnie commented Feb 25, 2018

Ok so I found out what the trouble was. (And I;m pretty surprised it didn't give any trouble on other platforms).
The test bot we use has the username ptb_test_bot which was only escaped properly on AppVeyor to ptb\_text\_bot Why this only happened on appveyor is a mystery for me. But it seems using text_html instead of text_markdown fixes the problem for this PR.

If CI finishes properly now it;s good to merge

@PaulSonOfLars

This comment has been minimized.

Show comment
Hide comment
@PaulSonOfLars

PaulSonOfLars Feb 26, 2018

Contributor

Awesome, glad I could help! And thanks for the debugging, I really had no clue what was going on there... Let me know if there's anything else that should be changed.

Contributor

PaulSonOfLars commented Feb 26, 2018

Awesome, glad I could help! And thanks for the debugging, I really had no clue what was going on there... Let me know if there's anything else that should be changed.

@tsnoam tsnoam merged commit b67ea7a into python-telegram-bot:master Mar 1, 2018

3 of 5 checks passed

codecov/patch 81.81% of diff hit (target 92.32%)
Details
codecov/project 92.13% (-0.2%) compared to b275031
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
hound No violations found. Woof!
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment