Skip to content

Input media subclasses init handling refactor #2573 #2717

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

Conversation

eldbud
Copy link
Contributor

@eldbud eldbud commented Oct 8, 2021

  • Refactored InputMedia.__init__ so that it initializes the attributes common to all deriving classes.
  • Add InputMedia._get_thumb_object static method
  • Created _BaseMedium class, base class for media objects
  • Created _BaseThumbedMedium class, base class for media objects that may have a thumbnail.
  • Refactored Media classes so that they inherit from _BaseMedium/_BaseThumbedMedium

closes #2573

Bibo-Joshi and others added 26 commits September 15, 2021 17:09
…ryptionError (python-telegram-bot#2621)

* move telegramdecryptionerror to error.py

* Change error class name
…t#2612)

* feat: add docs about docs

* fix: improve looks

* fix: make link work

* fix: this looks better

* Improved markdown, updated link

* Less justifying

Co-authored-by: Bibo-Joshi <22366557+Bibo-Joshi@users.noreply.github.com>
* Fix incomplete type annotations for CallbackContext

Co-authored-by: Hinrich Mahler <22366557+Bibo-Joshi@users.noreply.github.com>
* Feat: Custom pytest marker

Co-authored-by: Bibo-Joshi <22366557+Bibo-Joshi@users.noreply.github.com>
* Make basepersistence methods abstractmethod

Signed-off-by: starry69 <starry369126@outlook.com>

Co-authored-by: Hinrich Mahler <22366557+Bibo-Joshi@users.noreply.github.com>
…m-bot#2634)

Co-authored-by: Hinrich Mahler <22366557+Bibo-Joshi@users.noreply.github.com>
Co-authored-by: Harshil <37377066+harshil21@users.noreply.github.com>
Co-authored-by: Harshil <37377066+harshil21@users.noreply.github.com>
Co-authored-by: Harshil <37377066+harshil21@users.noreply.github.com>
Co-authored-by: poolitzer <25934244+Poolitzer@users.noreply.github.com>
# Conflicts:
#	telegram/bot.py
#	telegram/ext/callbackcontext.py
#	telegram/ext/dispatcher.py
#	telegram/ext/extbot.py
#	telegram/ext/jobqueue.py
#	tests/conftest.py
#	tests/test_bot.py
@Bibo-Joshi Bibo-Joshi self-requested a review October 8, 2021 16:49
@eldbud
Copy link
Contributor Author

eldbud commented Oct 9, 2021

the tests here are failing in pre-commit due to the

telegram/bot.py:232: error: unused 'type: ignore' comment
telegram/bot.py:233: error: unused 'type: ignore' comment
telegram/ext/extbot.py:162: error: unused 'type: ignore' comment
telegram/ext/extbot.py:163: error: unused 'type: ignore' comment
Found 4 errors in 2 files (checked 149 source files)

we discussed. any solution for this?

@Bibo-Joshi Bibo-Joshi mentioned this pull request Oct 9, 2021
5 tasks
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.

Hi. Thanks for the PR! I left a number of comments below. In addition to that:

  • IMO _Base(Thumb)Medium can both go into the same file basemedium.py
  • about mypy fails: in the reported lines we access something like input_media_object.parse_mode, but because we only know that isinstance(i_m_object, InputMedia) and up to now InputMedia has no attribute parse_mode, mypy threw an error. this is resolved by your PR, so the type: ignore comments are no longer needed - you can remove them
  • Please double check that the docs of Audio/Document etc show display the methods get_file and to/de_json (everything that's overridden in _Base(Thumb)Medium. The contribution guide has details on how to build the docs locally. You may need to add :inherited-members: to the files docs/source/telegram.audio/document/….rst, see here - 5th bullet point in "options and advanced usage"
  • I'm not entirely sure what's going on with the signature checks. Will have to look at that with more detail, which might take a few days
  • test_official is failing because InputMedia has no documented attributes, but we added some. Please add another exception to this logic

@eldbud
Copy link
Contributor Author

eldbud commented Oct 11, 2021

You probably need to convert the type hint into a string: def foo(… bot: 'Bot').

🤦‍♂️
of course, they're under if TYPE_CHECKING, foolishly forgot that. Thank you!

@eldbud
Copy link
Contributor Author

eldbud commented Oct 11, 2021

Ah, yes. test official only runs if you have an environment variable set. the contribution guide has some details about that :)

in the contribution guideline it says to run this: export TEST_OFFICIAL=true prior to running tests.
Because i am on windows computer, i replace "export" (which does not work in windows) with "set" (set TEST_OFFICIAL=true),
but it didn't help, i am still getting "test_official is not enabled".
What should i do?

@Bibo-Joshi Bibo-Joshi linked an issue Oct 12, 2021 that may be closed by this pull request
@Bibo-Joshi
Copy link
Member

Except for test_official, this LGTM now :) If you can't get the environment variable to work, you can also just temporarily delete the @pytest.mark.skipif decorator on test_official/test_official (at the very bottom). Note that pycharm also allows you to set environment configs via the run configurations.

@harshil21 harshil21 added enhancement 🛠 refactor change type: refactor labels Oct 12, 2021
@harshil21 harshil21 added this to the v14 milestone Oct 12, 2021
@eldbud
Copy link
Contributor Author

eldbud commented Oct 12, 2021

Except for test_official, this LGTM now :)

test_official is fixed.
last item left - documentation of attributes in subclasses

@eldbud
Copy link
Contributor Author

eldbud commented Oct 12, 2021

You can do something like

telegram.Audio
==============

.. Also lists methods of _Base(Thumb)Medium, but not the ones of TelegramObject

.. autoclass:: telegram.Audio
    :members:
    :show-inheritance:
    :inherited-members: TelegramObject

where the line .. Also lists … is a comment

  1. the ..autoclass::telegram.Audio line raises an error: C:\Users\eldad\PycharmProjects\python-telegram-bot\telegram\_files\audio.py:docstring of telegram._files.audio.Audio:1: WARNING: duplicate object description of telegram._files.audio.Audio, other instance in telegram.audio, use :noindex: for one of them

  2. without it, methods still don't appear.

@Bibo-Joshi can you maybe show me how the entire docstring should be written?

@Bibo-Joshi
Copy link
Member

@eldbud sorry, should have been more specific. The docs for the class telegram.Audio are generated by the file docs/source/telegram.audio.rst. You can adapat that file by adding the directive option :inherited-members: TelegramObject to the .. autoclass directive and by adding the comment. Similar for all other classes that inherit from _Base(Thumb)Medium

@eldbud
Copy link
Contributor Author

eldbud commented Oct 14, 2021

@eldbud sorry, should have been more specific. The docs for the class telegram.Audio are generated by the file docs/source/telegram.audio.rst. You can adapat that file by adding the directive option :inherited-members: TelegramObject to the .. autoclass directive and by adding the comment. Similar for all other classes that inherit from _Base(Thumb)Medium

@Bibo-Joshi i believe i am done. if not more comments, can this pr be approved?

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.

looks good!

@Bibo-Joshi Bibo-Joshi merged commit 38a6a6d into python-telegram-bot:v14 Oct 15, 2021
@Bibo-Joshi
Copy link
Member

Thanks for this contribution! :)

@eldbud eldbud deleted the InputMedia-subclasses-__init__-handling-refactor-#2573 branch October 15, 2021 22:04
@github-actions github-actions bot locked and limited conversation to collaborators Oct 17, 2021
@harshil21 harshil21 added the hacktoberfest-accepted other: hacktoberfest-accepted label Oct 30, 2021
@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.
Labels
🔌 enhancement pr description: enhancement hacktoberfest-accepted other: hacktoberfest-accepted 🛠 refactor change type: refactor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor how InputMedia* subclasses handle inits
7 participants