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

Bot API 3.3 #806

Merged
merged 6 commits into from Sep 1, 2017

Conversation

Projects
None yet
6 participants
@jeffffc
Copy link
Contributor

jeffffc commented Aug 23, 2017

Bot API 3.3
https://core.telegram.org/bots/api#august-23-2017

  • added forward_signature and author-signature to Message

  • added is_bot to User

  • added pinned_message to Chat

  • added mention() to telegram.utils.helpers for easy generation of inline text mentions with (id/User/Message) input

edit: i have no idea why it cannot import Message...

@jsmnbom

This comment has been minimized.

Copy link
Member

jsmnbom commented Aug 23, 2017

  1. You caused a circular import error please fix in __init__.py
  2. I'm not a fan of mention tbh. It should be either follow our convention of "_html" and "_markdown" or work like send_message and accept a parsemode.
  3. I also feel like it's weird that it accepts a Message object, what's the basis for that decision?
  4. Also why is the name overwriteable?
@Eldinnie
Copy link
Member

Eldinnie left a comment

I made some comments in the code.

Also I personally would prefer to do the mention a little different. A bit like @bomjacob suggested I would prefer it to be a helper within the telegram.User object.
So that you get:

user = telegram.User
user.mention_html == "<a href='tg:://user?self.id'>self.name</a>"

Users can then use
message.from_user.mention_markdown in their code for example.

@@ -57,6 +59,8 @@ class Chat(TelegramObject):
Returned only in get_chat.
invite_link (:obj:`str`, optional): Chat invite link, for supergroups and channel chats.
Returned only in get_chat.
pinned_message (:class:`telegram.Message`, optional): Pinned message, for supergroups.
Returned only in getChat.

This comment has been minimized.

@Eldinnie

Eldinnie Aug 23, 2017

Member

Please use get_chat

@@ -37,6 +37,8 @@ class Chat(TelegramObject):
photo (:class:`telegram.ChatPhoto`): Optional. Chat photo.
description (:obj:`str`): Optional. Description, for supergroups and channel chats.
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 getChat.

This comment has been minimized.

@Eldinnie

Eldinnie Aug 23, 2017

Member

please use get_chat

Optional if User or Message is provided and will overwrite the name in user.
Returns:
str:

This comment has been minimized.

@Eldinnie

Eldinnie Aug 23, 2017

Member

Please define return as :obj :str: The inline mention for the user as html or markdown.

if not user:
return None

if isinstance(user, Message):

This comment has been minimized.

@Eldinnie

Eldinnie Aug 23, 2017

Member

I would skip this, also see the total review comment.


elif isinstance(user, User):
if not name:
if user.last_name:

This comment has been minimized.

@Eldinnie

Eldinnie Aug 23, 2017

Member

The user object already has a name-property, wouldn;t it make more sense to use that?
https://github.com/python-telegram-bot/python-telegram-bot/blob/master/telegram/user.py#L67

order_info={})}
order_info={})},
{'forward_signature': 'some_forward_sign'},
{'author_signature': 'some_autohr_sign'}

This comment has been minimized.

@Eldinnie

Eldinnie Aug 23, 2017

Member

Spell mistake (although no big deal in tests, but still.

@Eldinnie

This comment has been minimized.

Copy link
Member

Eldinnie commented Aug 23, 2017

Hey @jeffffc . Thanks for your work!

I made a few adjustment requests in a code comment, Als o please see my suggestion regarding the mention helper. What do you think about that approach?
Also @bomjacob I like the way you can choose the name for the mention, say you want to rank players and name them first, second etc. in your message.

Lastly can you fix the import trouble? So we can see what Travis thinks about it.

Pieter

@jeffffc

This comment has been minimized.

Copy link
Contributor Author

jeffffc commented Aug 23, 2017

Let me answer and clarify before pushing updates.
@bomjacob
For 2, I will divide it into 2 functions then. For 3, yea i just thought that from my instinct, maybe that is not necessary, i will remove it. For 4, it was because for the text mention you do not necessarily use the real name of the user, for example you can make it as [I want something random](tg://user?id=123456789), but if you do not pass the name then it uses the user's name by default. I hope i made myself clear on this issue.
@Eldinnie
Sure It is sensible to make it into user object, i didnt think of that! For the User.name object yes I did notice the existence of it but as I wrote above, the text_mention can be showing manual-input strings but not a must to be its name. Moreover, for users who have usernames, User.name is default outputting their username, i don't when people using text_mentions on these users would want to show their username directly?

Thank you both for your response and I will surely fix the circular import issue.

@Eldinnie

This comment has been minimized.

Copy link
Member

Eldinnie commented Aug 23, 2017

@jeffffc you make a good point. I was merely saying that the fallback could be the name property, but I agree you should be able to set the name. So maybe properties for the mention_html/markdown isn't useful.

How about two helper methods within telegram.User

def mention_html(name=None) -> str (<a href ...>)
    
def mention_markdown(name=None) -> str ([name](...))
@jeffffc

This comment has been minimized.

Copy link
Contributor Author

jeffffc commented Aug 23, 2017

@Eldinnie sure.
Just one more thing atm: the new pinned_message is a message object within a chat object, while the current message object already imported chat object. How should i do to prevent the circular import? i tried to look it up but cant seems to find any good solution..

@Eldinnie

This comment has been minimized.

Copy link
Member

Eldinnie commented Aug 23, 2017

@jeffffc ouch, ehm, not at pc right now... I can look into it later.

jeffffc added some commits Aug 23, 2017

simpler helper methods (accept only custom names now)
attempt to fix circular import (not sure if its the correct way tho)
added helper methods into User object
@jeffffc

This comment has been minimized.

Copy link
Contributor Author

jeffffc commented Aug 23, 2017

The errors in travis tests should not be mine now.
So the changes now are:

  • User object
    --> User.mention_markdown(name=None) and User.mention_html(name=None)
    --> If name not passed, will use User.name by default

  • utils.helpers
    --> mention_markdown(user_id, name) and mention_html(user_id, name)
    --> assuming users will pass custom names

@Eldinnie

This comment has been minimized.

Copy link
Member

Eldinnie commented Aug 23, 2017

@jeffffc Looks good to me, also thanks for fixing all the tests. There's one flake8 error:
telegram/user.py:28:1: E303 too many blank lines (3)
Can you fix that last one? Then imo it's good to merge @tsnoam @jh0ker

@jeffffc

This comment has been minimized.

Copy link
Contributor Author

jeffffc commented Aug 23, 2017

you're welcome!
sure, i missed it my bad, just pushed a new commit.

@Eldinnie Eldinnie requested a review from tsnoam Aug 23, 2017

@jh0ker

This comment has been minimized.

Copy link
Member

jh0ker commented Aug 23, 2017

Looks good, thank you for your work :)

@jh0ker

jh0ker approved these changes Aug 23, 2017

@Eldinnie Eldinnie referenced this pull request Aug 23, 2017

Merged

Fix sticker tests #807

@Eldinnie

This comment has been minimized.

Copy link
Member

Eldinnie commented Aug 23, 2017

The tests failing are failing due to sticker trouble. #807 should fix those.

:obj:`str`: The inline mention for the user as HTML.
"""
if not name:
return util_mention_html(self.name, self.id)

This comment has been minimized.

@evgfilim1

evgfilim1 Aug 24, 2017

Contributor

You've mixed up arguments. First is user_id, second is name

This comment has been minimized.

@jeffffc

jeffffc Aug 24, 2017

Author Contributor

you are absolutely right. I mixed up with the old versions. thank you for spotting it out. pushing new commit now.

This comment has been minimized.

@Eldinnie

Eldinnie Aug 24, 2017

Member

oops, good spot.

if not name:
return util_mention_html(self.name, self.id)
else:
return util_mention_html(name, self.id)

This comment has been minimized.

@evgfilim1

evgfilim1 Aug 24, 2017

Contributor

See comment above

@Eldinnie Eldinnie changed the base branch from master to 7.1-branch Aug 26, 2017

@Eldinnie

This comment has been minimized.

Copy link
Member

Eldinnie commented Aug 26, 2017

Changed the base to 7.1-branch

@jsmnbom jsmnbom changed the base branch from 7.1-branch to master Aug 26, 2017

@Eldinnie Eldinnie merged commit b6a0853 into python-telegram-bot:master Sep 1, 2017

0 of 2 checks passed

coverage/coveralls Coverage decreased (-0.4%) to 90.595%
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@evgfilim1

This comment has been minimized.

Copy link
Contributor

evgfilim1 commented Sep 1, 2017

🎉

@jeffffc

This comment has been minimized.

Copy link
Contributor Author

jeffffc commented Sep 1, 2017

just wondering why travis keep saying build is in progress even all tests passed

@Eldinnie

This comment has been minimized.

Copy link
Member

Eldinnie commented Sep 1, 2017

@jeffffc I stopped travis to fix an error localy. Will push in a sec..

@jeffffc

This comment has been minimized.

Copy link
Contributor Author

jeffffc commented Sep 1, 2017

i see, thanks

@alexpacini

This comment has been minimized.

Copy link

alexpacini commented Oct 11, 2017

I suggest to edit the readme to reflect that API v3.3 is supported

@Eldinnie

This comment has been minimized.

Copy link
Member

Eldinnie commented Oct 11, 2017

@alexpacini Good point, although I hope we can skip that and just jump to 3.4 in a few days.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.