Skip to content

add caption, parse_mode, caption_entities, disable_web_page_preview parameters to Bot._send_message #3385

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.

Sign up for GitHub

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

Jump to bottom
Merged
Bibo-Joshi merged 7 commits into from
Nov 26, 2022
Merged

add caption, parse_mode, caption_entities, disable_web_page_preview parameters to Bot._send_message #3385

Bibo-Joshi merged 7 commits into from
Nov 26, 2022

Conversation

@lemontree210
Copy link
Member

@lemontree210 lemontree210 commented Nov 24, 2022 •
edited
Loading

closes #3372

Not sure I need to add tests (I think in this case I just needed to make sure existing tests don't fail).

@lemontree210 lemontree210 added the 🛠 refactor change type: refactor label Nov 24, 2022
@lemontree210 lemontree210 self-assigned this Nov 24, 2022
Bibo-Joshi
Bibo-Joshi requested changes Nov 24, 2022
View reviewed changes
@lemontree210
refactor: add docstring to ._send_message
a2370bc 
closes #3372
add doctstring and move comment there
@lemontree210 lemontree210 added enhancement and removed 🛠 refactor change type: refactor labels Nov 24, 2022
harshil21
harshil21 reviewed Nov 24, 2022
View reviewed changes
Copy link
Member

@harshil21 harshil21 left a comment

Choose a reason for hiding this comment

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

copy_message and edit_message_caption still have those if clauses left to remove.

Bibo-Joshi
Bibo-Joshi reviewed Nov 24, 2022
View reviewed changes
@lemontree210
Copy link
Member Author

lemontree210 commented Nov 24, 2022

copy_message and edit_message_caption still have those if clauses left to remove.

Yes, I agree on edit_message_caption(), but copy_message() calls Bot._post(), not Bot._send_message(), so not sure this change is applicable for copy_message()

@lemontree210
Copy link
Member Author

lemontree210 commented Nov 25, 2022

BTW a (maybe too) philosophical question. Can the fact that the generic ._send_message() method knows about other methods having parameters such as .caption (which pertain not to any message but to a specific type of message) be considered as breaking LoD? Maybe it's OK in this context because all of these params are part of official Telegram API.

@Bibo-Joshi
Copy link
Member

Bibo-Joshi commented Nov 25, 2022

fact that the generic ._send_message() method knows about other methods having parameters such as .caption

IMO this is not the case. _send_message just offers that parameter, but it doesn't really know if any other method actually uses it.

@lemontree210
add parse_mode to args of Bot._send_message()
be5cfef 
change all methods except for `.copy_message()`,
which doesn't call `._send_message()`
@lemontree210
Copy link
Member Author

lemontree210 commented Nov 25, 2022

Added parse_mode as mentioned in this comment for #3372

@lemontree210 lemontree210 changed the title refactor: add caption, caption_entities parameters to Bot._send_message refactor: add caption, parse_mode, caption_entities parameters to Bot._send_message Nov 25, 2022
@lemontree210 lemontree210 changed the title refactor: add caption, parse_mode, caption_entities parameters to Bot._send_message add caption, parse_mode, caption_entities parameters to Bot._send_message Nov 25, 2022
Bibo-Joshi
Bibo-Joshi reviewed Nov 25, 2022
View reviewed changes
@lemontree210 lemontree210 changed the title add caption, parse_mode, caption_entities parameters to Bot._send_message add caption, parse_mode, caption_entities, disable_web_page_preview parameters to Bot._send_message Nov 25, 2022
Bibo-Joshi
Bibo-Joshi approved these changes Nov 25, 2022
View reviewed changes
Copy link
Member

@Bibo-Joshi Bibo-Joshi left a comment

Choose a reason for hiding this comment

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

LGTM except for the comment below :)

@Bibo-Joshi Bibo-Joshi merged commit 637cc57 into master Nov 26, 2022
@Bibo-Joshi Bibo-Joshi deleted the caption-params-for-send-message-3372 branch November 26, 2022 18:05
@github-actions github-actions bot locked and limited conversation to collaborators Dec 4, 2022
@Bibo-Joshi Bibo-Joshi added 🔌 enhancement pr description: enhancement and removed enhancement labels Nov 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@Bibo-Joshi Bibo-Joshi Bibo-Joshi approved these changes

@harshil21 harshil21 harshil21 approved these changes

Assignees

@lemontree210 lemontree210

Labels

🔌 enhancement pr description: enhancement

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Add caption, caption_entities as parameters to Bot._send_message

4 participants

@lemontree210 @Bibo-Joshi @harshil21