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

Added methods to quickly reply to a message with Markdown or HTML #827

Merged
merged 14 commits into from
Feb 12, 2018

Conversation

JosXa
Copy link
Contributor

@JosXa JosXa commented Sep 11, 2017

I had problems mergin urllib3, so this PR closes #725.

@Eldinnie Indentation adjusted as per your request.

Tests are probably gonna have to wait until pytest stops messing up my workflow...

@codecov
Copy link

codecov bot commented Sep 14, 2017

Codecov Report

Merging #827 into master will decrease coverage by 0.15%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #827      +/-   ##
==========================================
- Coverage   91.83%   91.67%   -0.16%     
==========================================
  Files         103      103              
  Lines        4052     4061       +9     
  Branches      639      639              
==========================================
+ Hits         3721     3723       +2     
- Misses        193      197       +4     
- Partials      138      141       +3
Impacted Files Coverage Δ
telegram/message.py 96.36% <100%> (-0.63%) ⬇️
telegram/utils/request.py 67.85% <0%> (-0.9%) ⬇️
telegram/bot.py 87.51% <0%> (-0.5%) ⬇️

@Eldinnie
Copy link
Member

I edited the docstring and merged master. But it's still missing tests for these new methods.
Please add using monkeypatch. See test_message::test_reply_text for example

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.

But it's still missing tests for these new methods.
Please add using monkeypatch. See test_message::test_reply_text for example

@Eldinnie
Copy link
Member

@JosXa what is the status for this?

@JosXa
Copy link
Contributor Author

JosXa commented Oct 2, 2017

Tests added as discussed

@JosXa
Copy link
Contributor Author

JosXa commented Oct 2, 2017

Sorry, I keep committing files from other branches... Fixed now.

@JosXa JosXa self-assigned this Oct 7, 2017
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.

Some small docstring improvements and please restore the dict in test_message to the state it was.

Other than that it looks good to me.

def reply_markdown(self, *args, **kwargs):
"""Shortcut for::

bot.sendMessage(update.message.chat_id, parse_mode=ParseMode.MARKDOWN, *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.

Please use the snake_case names in docs. (send_message)

def reply_html(self, *args, **kwargs):
"""Shortcut for::

bot.sendMessage(update.message.chat_id, parse_mode=ParseMode.HTML, *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.

same as above

{'forward_from_chat': Chat(-23, 'channel'),
'forward_from_message_id': 101,
'forward_date': datetime.now()},
{
Copy link
Member

Choose a reason for hiding this comment

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

Please reset the dict to the way it was.

@tsnoam
Copy link
Member

tsnoam commented Feb 10, 2018

The code itself LGTM.

However, I think it's over-engineering and only bloats the code. Though if the other maintainers find it suitable for ptb, I won't object.

@tsnoam
Copy link
Member

tsnoam commented Feb 12, 2018

@jh0ker thinks it a good idea to have this in the library. merging.

@tsnoam tsnoam merged commit f0dfdfb into master Feb 12, 2018
@tsnoam tsnoam deleted the reply-text-markdown-2 branch February 12, 2018 13:39
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