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

support 3.4 API #865

Merged
merged 15 commits into from
Oct 14, 2017
Merged

support 3.4 API #865

merged 15 commits into from
Oct 14, 2017

Conversation

Eldinnie
Copy link
Member

WIP
Live location stuff added See #864

@codecov
Copy link

codecov bot commented Oct 11, 2017

Codecov Report

Merging #865 into master will decrease coverage by 0.13%.
The diff coverage is 88.73%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #865      +/-   ##
==========================================
- Coverage   91.89%   91.76%   -0.14%     
==========================================
  Files         101      101              
  Lines        3987     4055      +68     
  Branches      603      620      +17     
==========================================
+ Hits         3664     3721      +57     
- Misses        187      196       +9     
- Partials      136      138       +2
Flag Coverage Δ
#Appveyor 86.88% <88.73%> (+0.02%) ⬆️
#Travis 91.34% <85.91%> (-0.18%) ⬇️
Impacted Files Coverage Δ
telegram/inline/inputlocationmessagecontent.py 100% <100%> (ø) ⬆️
telegram/inline/inlinequeryresultlocation.py 100% <100%> (ø) ⬆️
telegram/message.py 96.22% <100%> (+0.21%) ⬆️
telegram/chat.py 100% <100%> (ø) ⬆️
telegram/bot.py 87.22% <84.31%> (-0.79%) ⬇️
telegram/ext/updater.py 76.66% <0%> (ø) ⬆️
telegram/utils/request.py 67.85% <0%> (+0.89%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 445bcde...f39b403. Read the comment docs.

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

Ok I think I got everything in. Something weird going on with test_official. I don't really get the error. I'm hoping @bomjacob can find time to figure it out.

Also we need to let the bot set a stickerset on our group every test (going to be problematic I'm affraid) to properly test the set_chat_sticker_set and delete_chat_sticker_set methods.

Review wanted.

@Eldinnie
Copy link
Member Author

I found the problem with test_official.
It seems TG has their table messed up on:
https://core.telegram.org/bots/api#inputlocationmessagecontent
and
https://core.telegram.org/bots/api#inlinequeryresultlocation

Copy link
Member

@tsnoam tsnoam left a comment

Choose a reason for hiding this comment

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

LGTM
Just one thing - because of the issue with the test_official, we must fix or skip the test (I wouldn't count on them to fix their documentation). Otherwise, our tests will keep failing.

@Eldinnie
Copy link
Member Author

Note from @jh0ker Very nice, although perhaps it would be good to mention in the docstring that you can pass either location or lat/lon to edit_message_live_location

@Eldinnie Eldinnie changed the title started work on 3.4 API support 3.4 API Oct 11, 2017
Copy link
Member

@tsnoam tsnoam left a comment

Choose a reason for hiding this comment

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

All changes looks good.
However, the thing with a 2nd CR is that you see the things you haven't seen the first time.
I think that location should be mutually exclusive to (longitude, latitude). You can do something like to make sure that one and only one of the two is supplied:

if not (None not in (longitude, latitude)) ^ bool(location):
    raise Exception...

@Eldinnie
Copy link
Member Author

Need to up tests aswell. Hopefully have time tomorrow

@Eldinnie
Copy link
Member Author

@tsnoam added the requested check (and tests). If we can live with a small decrease in coverage due to not testing the groupstickerset stuff I think it's ready to merge/release

@Eldinnie
Copy link
Member Author

@tsnoam
if not (latitude is not None or longitude is not None) ^ bool(location):
Ugly, but works.
If location and either lat or lon is not None it will raise.

@tsnoam
Copy link
Member

tsnoam commented Oct 12, 2017

@Eldinnie As discussed, please add () around the entire logical expression which start just after if not.

@Eldinnie
Copy link
Member Author

@tsnoam Done

Copy link
Member

@tsnoam tsnoam left a comment

Choose a reason for hiding this comment

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

LGTM
This passes Travis/appveyor and we can merge.

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.

LGTM, tiny nitpick below, but it's so small we probably could just merge ^^

telegram/chat.py Outdated
@@ -38,6 +38,9 @@ class Chat(TelegramObject):
invite_link (:obj:`str`): Optional. Chat invite link, for supergroups and channel chats.
pinned_message (:class:`telegram.Message`): Optional. Pinned message, for supergroups.
Returned only in get_chat.
sticker_set_name (:obj:`str`): Optional. For supergroups, name of Group sticker set.
can_set_sticker_set (:obj:`bool`): Optional. ``True``, if the bot can change group the
Copy link
Member

Choose a reason for hiding this comment

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

I think a tab character snuck in here

Copy link
Member Author

Choose a reason for hiding this comment

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

it did will fix

telegram/chat.py Outdated
@@ -61,6 +64,10 @@ class Chat(TelegramObject):
pinned_message (:class:`telegram.Message`, optional): Pinned message, for supergroups.
Returned only in get_chat.
bot (:class:`telegram.Bot`, optional): The Bot to use for instance methods.
sticker_set_name (:obj:`str`, optional): For supergroups, name of Group sticker set.
Returned only in get_chat.
can_set_sticker_set (:obj:`bool`, optional): ``True``, if the bot can change group the
Copy link
Member

Choose a reason for hiding this comment

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

Same here

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.

None yet

3 participants