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

Make chat_id a positional argument inside shortcut methods of Chat and User classes #1048 #1050

Merged
merged 2 commits into from Mar 16, 2018

Conversation

Projects
None yet
3 participants
@xates
Contributor

xates commented Mar 16, 2018

No description provided.

@jh0ker

jh0ker approved these changes Mar 16, 2018

Looks good to me, no problems with backwards compatibility I can see. Should be good to merge

@jh0ker

This comment has been minimized.

Show comment
Hide comment
@jh0ker

jh0ker Mar 16, 2018

Member

@xates Looks like you also have to update the tests

Member

jh0ker commented Mar 16, 2018

@xates Looks like you also have to update the tests

@xates

This comment has been minimized.

Show comment
Hide comment
@xates

xates Mar 16, 2018

Contributor

Ok, I have tried to figure out how they work but I am not entirely sure... would something like that work for test_chat.py for example?

    def test_instance_method_send_message(self, monkeypatch, chat):
        def test(*args, **kwargs):
            return args[0] == chat.id and args[1] == 'test'

        monkeypatch.setattr('telegram.Bot.send_message', test)
        assert chat.send_message('test')

    def test_instance_method_send_audio(self, monkeypatch, chat):
        def test(*args, **kwargs):
            return args[0] == chat.id and args[1] == 'test_audio'

        monkeypatch.setattr('telegram.Bot.send_audio', test)
        assert chat.send_audio('test_audio')

Moreover, I have noticed that both Chat and User classes lack the tests for send_photo. Is that intentional or should I add them?

Contributor

xates commented Mar 16, 2018

Ok, I have tried to figure out how they work but I am not entirely sure... would something like that work for test_chat.py for example?

    def test_instance_method_send_message(self, monkeypatch, chat):
        def test(*args, **kwargs):
            return args[0] == chat.id and args[1] == 'test'

        monkeypatch.setattr('telegram.Bot.send_message', test)
        assert chat.send_message('test')

    def test_instance_method_send_audio(self, monkeypatch, chat):
        def test(*args, **kwargs):
            return args[0] == chat.id and args[1] == 'test_audio'

        monkeypatch.setattr('telegram.Bot.send_audio', test)
        assert chat.send_audio('test_audio')

Moreover, I have noticed that both Chat and User classes lack the tests for send_photo. Is that intentional or should I add them?

@jh0ker

This comment has been minimized.

Show comment
Hide comment
@jh0ker

jh0ker Mar 16, 2018

Member

I believe that will work, but please confirm it locally before committing (as per contributing guide).

And yes, that looks like an oversight, adding tests for send_photo would be appreciated 👍

Member

jh0ker commented Mar 16, 2018

I believe that will work, but please confirm it locally before committing (as per contributing guide).

And yes, that looks like an oversight, adding tests for send_photo would be appreciated 👍

@xates

This comment has been minimized.

Show comment
Hide comment
@xates

xates Mar 16, 2018

Contributor

I cannot run the tests locally because I get an ImportError: No module named ptb_urllib3.urllib3

Contributor

xates commented Mar 16, 2018

I cannot run the tests locally because I get an ImportError: No module named ptb_urllib3.urllib3

@Eldinnie

This comment has been minimized.

Show comment
Hide comment
@Eldinnie

Eldinnie Mar 16, 2018

Member

@xates Looks like you didn't follow this step from the contributing guide:

Clone your forked repository of python-telegram-bot to your computer:

$ git clone https://github.com/<your username>/python-telegram-bot --recursive

To fix it run:

$ git submodule update --init --recursive
Member

Eldinnie commented Mar 16, 2018

@xates Looks like you didn't follow this step from the contributing guide:

Clone your forked repository of python-telegram-bot to your computer:

$ git clone https://github.com/<your username>/python-telegram-bot --recursive

To fix it run:

$ git submodule update --init --recursive
@jh0ker

This comment has been minimized.

Show comment
Hide comment
@jh0ker

jh0ker Mar 16, 2018

Member

@xates Sounds like you didn't clone with --recursive to also clone git submodules (as per contribution guide)
beat me to it

Member

jh0ker commented Mar 16, 2018

@xates Sounds like you didn't clone with --recursive to also clone git submodules (as per contribution guide)
beat me to it

@xates

This comment has been minimized.

Show comment
Hide comment
@xates

xates Mar 16, 2018

Contributor

You're right, sorry, I missed that --recursive that was at the end

Contributor

xates commented Mar 16, 2018

You're right, sorry, I missed that --recursive that was at the end

@jh0ker

This comment has been minimized.

Show comment
Hide comment
@jh0ker

jh0ker Mar 16, 2018

Member

If you got errors in test_get_chat you can ignore them

Member

jh0ker commented Mar 16, 2018

If you got errors in test_get_chat you can ignore them

@xates

This comment has been minimized.

Show comment
Hide comment
@xates

xates Mar 16, 2018

Contributor

Oh, I see! That was driving me crazy...

Contributor

xates commented Mar 16, 2018

Oh, I see! That was driving me crazy...

@jh0ker

This comment has been minimized.

Show comment
Hide comment
@jh0ker

jh0ker Mar 16, 2018

Member

I'm sorry about that, someone unwittingly renamed the group that is used in that test so the test suddenly failed everywhere.

Member

jh0ker commented Mar 16, 2018

I'm sorry about that, someone unwittingly renamed the group that is used in that test so the test suddenly failed everywhere.

@jh0ker

This comment has been minimized.

Show comment
Hide comment
@jh0ker

jh0ker Mar 16, 2018

Member

Looks good to me 👍

Member

jh0ker commented Mar 16, 2018

Looks good to me 👍

@jh0ker jh0ker merged commit 3ccf40e into python-telegram-bot:master Mar 16, 2018

1 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
hound No violations found. Woof!
@jh0ker

This comment has been minimized.

Show comment
Hide comment
@jh0ker

jh0ker Mar 16, 2018

Member

Thank you very much for your contribution :)

Member

jh0ker commented Mar 16, 2018

Thank you very much for your contribution :)

@xates

This comment has been minimized.

Show comment
Hide comment
@xates

xates Mar 16, 2018

Contributor

Yeah, I guess that SSL problem is not my fault 😄

Well, thank you, for running such a wonderful project 😉

Contributor

xates commented Mar 16, 2018

Yeah, I guess that SSL problem is not my fault 😄

Well, thank you, for running such a wonderful project 😉

@xates xates deleted the xates:positional-args branch Mar 16, 2018

@jh0ker

This comment has been minimized.

Show comment
Hide comment
@jh0ker

jh0ker Mar 16, 2018

Member

Eh, tests sometimes pass and fail depending on the alignment of the planets 🌑

Member

jh0ker commented Mar 16, 2018

Eh, tests sometimes pass and fail depending on the alignment of the planets 🌑

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment