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 t.me links for User, Chat and Message if available and update User.mention_* #1092

Merged
merged 38 commits into from
May 9, 2018

Conversation

jonowo
Copy link
Contributor

@jonowo jonowo commented Apr 27, 2018

  • User.link (t.me link of a user if the user has a username)
  • Chat.link (t.me link of a supergroup if it has a username)
  • Message.link (t.me link of a message if it is in a supergroup with a username)

Updated User.mention_markdown and User.mention_html so that the default value of name is User.full_name instead of User.name. If a user has a username, User.name returns the user's username prefixed with "@". It makes no sense to markdown a username, since the username already mentions the user.

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.

I like the idea a lot! Not sure how the other feel yet though. Please see attached comments though.

telegram/chat.py Outdated
:attr:`username`, returns a t.me link of the chat.
"""
if self.type == "supergroup" and self.username:
return "t.me/{}".format(self.usermame)
Copy link
Member

Choose a reason for hiding this comment

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

Should this default to something? (if it's not a supergroup)

telegram/chat.py Outdated
:obj:`str`: Convenience property. If the chat is a supergroup and has a
:attr:`username`, returns a t.me link of the chat.
"""
if self.type == "supergroup" and self.username:
Copy link
Member

Choose a reason for hiding this comment

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

Please use the Chat.SUPERGROUP attribute here.
(comment applies elsewhere too)

if not name:
return util_mention_markdown(self.id, self.name)
else:
if name:
Copy link
Member

Choose a reason for hiding this comment

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

This change is out of scope... we're more likely to merge simple self contained pull requests...
IMO this is fine this time though :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will separate into 2 PRs next time.

Copy link
Member

@Eldinnie Eldinnie left a comment

Choose a reason for hiding this comment

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

Basically the changes look good. Some small comments on docstrings. However tests are missing. Can you please modify/change the tests so they won't fail and diff coverage is 100%
After that we can merge

telegram/user.py Outdated
@@ -104,7 +113,7 @@ def get_profile_photos(self, *args, **kwargs):
"""
Shortcut for::

bot.get_user_profile_photos(update.message.from_user.id, *args, **kwargs)
Copy link
Member

Choose a reason for hiding this comment

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

Revert this indent please. Otherwise docs will build weirdly

telegram/user.py Outdated
@@ -124,28 +133,24 @@ def de_list(cls, data, bot):
def mention_markdown(self, name=None):
"""
Args:
name (:obj:`str`): If provided, will overwrite the user's name.

name (:obj:`str`): If provided, will overwrite the user's full name.
Copy link
Member

Choose a reason for hiding this comment

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

Add the blank line between the headers again. Without it docs will mess up.

telegram/user.py Outdated

def mention_html(self, name=None):
"""
Args:
name (:obj:`str`): If provided, will overwrite the user's name.

name (:obj:`str`): If provided, will overwrite the user's full name.
Copy link
Member

Choose a reason for hiding this comment

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

Add the blank line between the headers again. Without it docs will mess up.

@jonowo
Copy link
Contributor Author

jonowo commented Apr 30, 2018

Fixed docstrings. Now working on tests.

@tsnoam
Copy link
Member

tsnoam commented May 7, 2018

@Tr-Jono
what is the status of this?

@jonowo
Copy link
Contributor Author

jonowo commented May 7, 2018

Modified tests for user and chat. Will work on test_message.py later.

@jonowo jonowo closed this May 8, 2018
@jonowo jonowo reopened this May 8, 2018
@jonowo
Copy link
Contributor Author

jonowo commented May 8, 2018

Fixed mistakes, waiting for the checks. I could only work on mobile...

jeffffc and others added 4 commits May 9, 2018 12:21
…p for its link, message links should be available for channels too, fix typos, add more tests for links, confirmed tests passed
Fixed and added more tests, minor change on chat/message link functions
fix docstrings blank lines
@jonowo
Copy link
Contributor Author

jonowo commented May 9, 2018

@jeffffc helped fixed the tests & docstrings, and include links for channels. Should be fine now. ☺

@Eldinnie
Copy link
Member

Eldinnie commented May 9, 2018

Hey @Tr-Jono . Did you install the pre-commit hook like explained in our Contributing guide

Flake8 found a problem which makes CI fail:

telegram/user.py:152:1: W293 blank line contains whitespace

@jeffffc
Copy link
Contributor

jeffffc commented May 9, 2018

It was my bad, missed the pre-commit line when I set up a new venv. Will push again a while later, sorry for the silly mistakes we made.

@Eldinnie
Copy link
Member

Eldinnie commented May 9, 2018

No problem :D

fix docstrings whitespaces in blank lines
@jeffffc
Copy link
Contributor

jeffffc commented May 9, 2018

Good to go!

Some docstring formatting.
Removed some redundant tests
@Eldinnie
Copy link
Member

Eldinnie commented May 9, 2018

i pushed some last fixes and it's good to go with that IMO. The patch fix being 75% is due to missing tests for mention_html and `mention_markdown in the first place. Will fix that in another PR.

@Eldinnie
Copy link
Member

Eldinnie commented May 9, 2018

@Tr-Jono one last thing. If you like you can add yourself to the contributers list.
Just commit or reply here with the entry you want in there and I'll add it when I merge.

EDIT:
NVm you're already on there :D

@Eldinnie Eldinnie merged commit 94abf16 into python-telegram-bot:master May 9, 2018
@Eldinnie
Copy link
Member

Eldinnie commented May 9, 2018

Thank you for your contribution

Eldinnie pushed a commit that referenced this pull request May 31, 2018
…r.mention_* (#1092)

* Add User.link and update User.mention_*
* Add Chat.link
* Add Message.link
* Link returns None on default
* Add test link
Eldinnie pushed a commit that referenced this pull request May 31, 2018
…r.mention_* (#1092)

* Add User.link and update User.mention_*
* Add Chat.link
* Add Message.link
* Link returns None on default
* Add test link
@github-actions github-actions bot locked and limited conversation to collaborators Aug 21, 2020
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

5 participants