Skip to content

Properly split and handle arguments in CommandHandler #414

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.

Sign up for GitHub

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

Jump to bottom
Merged
jh0ker merged 3 commits into from
Sep 20, 2016
Merged

Properly split and handle arguments in CommandHandler #414

jh0ker merged 3 commits into from
Sep 20, 2016

Conversation

@eligao
Copy link
Contributor

@eligao eligao commented Sep 18, 2016

Assume we got a command with more than one continuous white-space characters (which does happen) :

/func[space][space]arg1[space]arg2[space][space]arg3[space][space][space]arg4

after split(' '), we have
['/func', '', 'arg1', 'arg2', '', 'arg3', '', '', 'arg4']
which includes many unnecessary empty strings that leads to improper command handling.
This is also different from how the system handles standard argv[].
However with split(), we have the proper parameters:
['/func', 'arg1', 'arg2', 'arg3', 'arg4']
Also it eliminates other unnecessary white-spaces like \r,\n, etc.

jsmnbom
jsmnbom reviewed Sep 18, 2016
View reviewed changes
Copy link
Member

@jsmnbom jsmnbom left a comment

Choose a reason for hiding this comment

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

Seems like the expected behaviour to me.
The only problem I could see with this is that I've seen a lot of people assume that they can get the whole argument using ' '.join(args), which no longer would be true..

jsmnbom
jsmnbom requested changes Sep 18, 2016
View reviewed changes
Copy link
Member

@jsmnbom jsmnbom left a comment

Choose a reason for hiding this comment

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

Please change the docstring for the pass_args argument to fit the change. Other than that, it looks good :)

@eligao
Copy link
Contributor Author

eligao commented Sep 18, 2016

But after this commit, one could get a "regularized" version of the original argument.
Also they could probably use update.message.text to get the whole original argument.

>>> s='/func   1 2  3 4'
>>> ' '.join(s.split(' ')[1:])
'   1 2  3 4'
>>> ' '.join(s.split()[1:])
'1 2 3 4'
>>>

@jsmnbom
Copy link
Member

jsmnbom commented Sep 18, 2016

Yeah that's true. Please address the docstring change I reviewed above and it looks good to go as far as I'm concerned.

@eligao
Copy link
Contributor Author

eligao commented Sep 18, 2016

Noted :)

@eligao
Copy link
Contributor Author

eligao commented Sep 18, 2016

I've updated the docstring in CommandHandler.
Also I did the same change in StringCommandHandler because they look similar, but I'm not quite sure if it looks good because I never used this class.

jh0ker
jh0ker approved these changes Sep 20, 2016
View reviewed changes
Copy link
Member

@jh0ker jh0ker left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@jh0ker jh0ker added this to the 5.1 milestone Sep 20, 2016
@jh0ker jh0ker merged commit a91fe5f into python-telegram-bot:master Sep 20, 2016
@eligao eligao deleted the whitespace_args branch September 20, 2016 05:51
@github-actions github-actions bot locked and limited conversation to collaborators Aug 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@jsmnbom jsmnbom jsmnbom requested changes

@jh0ker jh0ker jh0ker approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

5.1

Development

Successfully merging this pull request may close these issues.

3 participants

@eligao @jsmnbom @jh0ker