Conversation
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
|
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 Review wanted. |
|
I found the problem with test_official. |
tsnoam
left a comment
There was a problem hiding this comment.
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.
|
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 |
There was a problem hiding this comment.
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...
|
Need to up tests aswell. Hopefully have time tomorrow |
|
@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 |
|
@tsnoam |
|
@Eldinnie As discussed, please add () around the entire logical expression which start just after if not. |
|
@tsnoam Done |
tsnoam
left a comment
There was a problem hiding this comment.
LGTM
This passes Travis/appveyor and we can merge.
jsmnbom
left a comment
There was a problem hiding this comment.
LGTM, tiny nitpick below, but it's so small we probably could just merge ^^
telegram/chat.py
Outdated
| 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 |
There was a problem hiding this comment.
I think a tab character snuck in here
telegram/chat.py
Outdated
| 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 |
[ci skip]
WIP
Live location stuff added See #864