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

add support for 3.5 api #920

Merged
merged 14 commits into from
Dec 8, 2017
Merged

add support for 3.5 api #920

merged 14 commits into from
Dec 8, 2017

Conversation

Eldinnie
Copy link
Member

@Eldinnie Eldinnie commented Nov 21, 2017

As discussed do the current InputMedia* throw a ValueError when supplied with a file instead of a file_id or and url.

This is because the current workings of InputFile and it's mechanics to encode as multipart form data don't support multiple files. And quite frankly, I have no Idea how to do that.

Closes #918

@python-telegram-bot python-telegram-bot deleted a comment from codecov bot Nov 22, 2017
@python-telegram-bot python-telegram-bot deleted a comment from codecov bot Nov 22, 2017
@python-telegram-bot python-telegram-bot deleted a comment from codecov bot Nov 22, 2017
@python-telegram-bot python-telegram-bot deleted a comment from codecov bot Nov 22, 2017
@Eldinnie
Copy link
Member Author

Review wanted.

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.

Looks really good!
Some small quick notes :)
Then we need to decide if we're gonna release it like this or if we need to get sending new files in groups to work first.

if isinstance(media, PhotoSize):
self.media = media.file_id
elif hasattr(media, 'read'):
raise ValueError('We only support file_id or url as a valid media. Sending files is '
Copy link
Member

Choose a reason for hiding this comment

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

Maybe add a TODO: about this?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, will do

@@ -0,0 +1,31 @@
#!/usr/bin/env python
Copy link
Member

Choose a reason for hiding this comment

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

Should these files maybe be in files/?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, maybe.


def test_with_video(self, video):
# fixture found in test_video
imv = InputMediaVideo(video, caption="test 3")
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we use abbreviations in the other tests

Copy link
Member Author

Choose a reason for hiding this comment

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

We don;t and I'm lazy, will fix

assert isinstance(messages, list)
assert len(messages) == 2
assert all([isinstance(mes, Message) for mes in messages])
assert all([mes.media_group_id == messages[0].media_group_id for mes in messages])
Copy link
Member

Choose a reason for hiding this comment

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

Maybe a TODO: about needing to test with new files.

Copy link
Member Author

Choose a reason for hiding this comment

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

yup

@@ -107,6 +107,8 @@ def check_object(h4):
field = 'from_user'
elif name.startswith('InlineQueryResult') and field == 'type':
continue
elif name.startswith('InputMedia') and field == 'type':
Copy link
Member

Choose a reason for hiding this comment

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

Could you combine with above?
elif (name.startswith('InlineQueryResult') or name.startswith('InputMedia')) and field == 'type':
seems cleaner to me.

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, I pondered on it, but I will

assert photo.width == 1920
assert photo.height == 1080
assert photo.file_size == 30907
assert photo.width == 1280
Copy link
Member

Choose a reason for hiding this comment

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

What changed here?
And how... like it's a smaller image but it takes up more space? :/

Copy link
Member Author

Choose a reason for hiding this comment

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

I have no Idea, but it's what came back...

@python-telegram-bot python-telegram-bot deleted a comment from codecov bot Dec 5, 2017
@python-telegram-bot python-telegram-bot deleted a comment from codecov bot Dec 5, 2017
@python-telegram-bot python-telegram-bot deleted a comment from codecov bot Dec 5, 2017
@python-telegram-bot python-telegram-bot deleted a comment from codecov bot Dec 5, 2017
@python-telegram-bot python-telegram-bot deleted a comment from codecov bot Dec 5, 2017
@Eldinnie
Copy link
Member Author

Eldinnie commented Dec 5, 2017

I addressed everything @bomjacob pointed out. As discussed I think we should merge and release without the option to send new files yet.

Appveyor will fail due to python 3.3 drop in support from pytest. As will we (see #930) Good to go?

@python-telegram-bot python-telegram-bot deleted a comment from codecov bot Dec 5, 2017
@python-telegram-bot python-telegram-bot deleted a comment from codecov bot Dec 5, 2017
elif name.startswith('InlineQueryResult') and field == 'type':
continue
elif name.startswith('InputMedia') and field == 'type':
elif (name.startswith('InlineQueryResult') or name.startswith('InputMedia')) \
Copy link
Member

Choose a reason for hiding this comment

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

Could you wrap it all in paranthesis instead of using the backslash here maybe?, doesn't look very clean IMO

Copy link
Member Author

Choose a reason for hiding this comment

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

👍

@python-telegram-bot python-telegram-bot deleted a comment from codecov bot Dec 6, 2017
@python-telegram-bot python-telegram-bot deleted a comment from codecov bot Dec 6, 2017
Make it possible to send an object that will be json-serialized for send_invoice + tests
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New API changes without changelog
2 participants