Skip to content

Properly split and handle arguments in CommandHandler - #414

Merged
jh0ker merged 3 commits into
python-telegram-bot:masterfrom
eligao:whitespace_args
Sep 20, 2016
Merged

Properly split and handle arguments in CommandHandler#414
jh0ker merged 3 commits into
python-telegram-bot:masterfrom
eligao:whitespace_args

Conversation

@eligao

@eligao eligao commented Sep 18, 2016

Copy link
Copy Markdown
Contributor

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

@jsmnbom jsmnbom left a comment

Copy link
Copy Markdown
Member

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

@jsmnbom jsmnbom left a comment

Copy link
Copy Markdown
Member

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

eligao commented Sep 18, 2016

Copy link
Copy Markdown
Contributor Author

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

jsmnbom commented Sep 18, 2016

Copy link
Copy Markdown
Member

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

eligao commented Sep 18, 2016

Copy link
Copy Markdown
Contributor Author

Noted :)

@eligao

eligao commented Sep 18, 2016

Copy link
Copy Markdown
Contributor Author

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

@jh0ker jh0ker left a comment

Copy link
Copy Markdown
Member

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