Skip to content

Conversation

@lemontree210
Copy link
Member

@lemontree210 lemontree210 commented Oct 16, 2022

closes #3185

Adds 3 keyword-only arguments to pass caption to entire media group.

All tests pass (locally) including test_official.py.

All pre-commit hooks passed except for pylint that found errors in code that I hadn't changed (cyclic import in telegram/ext/_chatjoinrequesthandler.py).

I'm not sure if I have to:

  • Add.. versionadded:: version, .. versionchanged:: version or .. deprecated:: version to the docstrings for user facing changes (for methods/class descriptions, arguments and attributes) (I changed doctrings themselves, just not sure about these version... notes)
  • Add more tests, e.g. for shortcuts (Chat, User, Message)

closes #3185

add 3 keyword-only arguments to pass caption
to entire media group
before that a caption for the group had to be
manually added to the 1st media item in the group
@Bibo-Joshi Bibo-Joshi self-requested a review October 16, 2022 20:48
"group_caption", "group_caption_parse_mode",
and "group_caption_entities" are not part of API,
so exclude them from check of arguments matching API
@lemontree210
Copy link
Member Author

lemontree210 commented Oct 17, 2022

Just a side note: only one check failed for penultimate commit 07b175e (test_official.py failed), so I don't think I introduced something that caused so many tests to fail in my last commit (0cdfd92).

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.

Thank you very much for your nice PR! I left a number of comments - but most are actually just nit-picking on the documentation :D

- replace "images" with "media"
- check not only .caption but also .caption_entities
and .parse_mode of individual media items
we should try to copy as little as possible:
if none of the `group_*` parameters was passed,
we don't need copying.

Otherwise, we only need to copy (the list and)
the first media item rather than all of them
rename to match arguments of functions that handle
individual media:

- group_caption -> caption
- group_caption_parse_mode -> parse_mode
- group_caption_entities -> caption_entities
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.

Thank you for the updates! I left a few more comments below. Take your time with the defaults stuff (nice to see that you're eager to figure it out yourself!), but feel free to reach out for help if you want it :)

check that the error is raised if user passes
media that only have `parse_mode` or
`caption_entities`
Check if the user explicitly set parse_mode
for the InputMedia* object.
Even if it is set to None, ValueError must be
raised.
add fixture for a media group without individual
captions rather than taking `media_group` fixtures
and setting attributes manually

this fixture can be used later when checking
defaults
* make `parse_mode` default to DEFAULT_NONE
in `Bot` and shortcut methods in classes
* exclude `parse_mode` from check in conftest.py
* add tests for handling of default values
to allow multiple calls with same media group,
I need to copy not only the first item
(that gets the caption) but all items because
otherwise bot with Defaults will assign its
default parse mode to every item,
which will cause ValueError upon subsequent calls

unless there are other things to be fixed,
this should be the commit that closes #3185
@Bibo-Joshi Bibo-Joshi added hacktoberfest-accepted other: hacktoberfest-accepted 📋 do-not-merge-yet work status: do-not-merge-yet labels Oct 23, 2022
also remove `timeout` as a remnant from v13
`_bot.py` had two imports (import copy and from copy import copy)

leave first import and replace `copy()` with `copy.copy()`
in code
Closes #3185

Refactoring becomes possible since #3305 was fixed.

Refactoring is based on ideas in commit
c38801f
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.

Awesome 🥳

@Bibo-Joshi Bibo-Joshi merged commit d2c6c4b into python-telegram-bot:master Nov 2, 2022
@Bibo-Joshi
Copy link
Member

Thanks again for your work & patience on this feature :)

@lemontree210
Copy link
Member Author

Thank you! I hope we didn't forget to add versionadded to the docstring. I mean, I know we didn't add it but I don't know whether we should have 😆

@lemontree210 lemontree210 deleted the caption-for-media-group-3185 branch November 2, 2022 07:54
@Bibo-Joshi
Copy link
Member

Ah, damn. I'll add that in the doc-fixes branch :)

@github-actions github-actions bot locked and limited conversation to collaborators Nov 9, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

📋 do-not-merge-yet work status: do-not-merge-yet hacktoberfest-accepted other: hacktoberfest-accepted

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[FEATURE] Add caption parameter to functions that consume a media group

2 participants