Skip to content

Builder pattern for ext #2646

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

Merged
merged 87 commits into from
Oct 9, 2021
Merged

Builder pattern for ext #2646

merged 87 commits into from
Oct 9, 2021

Conversation

Bibo-Joshi
Copy link
Member

@Bibo-Joshi Bibo-Joshi commented Aug 31, 2021

Closes #2556

Notes:

  • I did not add an ExtBotBuilder class. Currently only the request argument of ExtBot would profit from with. Then again, the init of ExtBot may become more complicated in the future if we add more features. If we want to add a corresponding class, we need to think about if we still want to allow for manual instantiation of ExtBot or not …
  • I made sure that Updater can be used without Dispatcher. IMO this emphasizes that both have different tasks and for advanced users it may be helpful to outsource the fetching of updates while doing the processing manually.
  • In the same spirit I made Dispatcher/Updater.exception_event properly documented attributes and they are no longer protected. While exception_event was probably initially meant as means for private communication between Updater and Dispatcher I do feel like it could be useful to have it more publically available for advaced, custom setups.
  • I introduced a new type alias DefaultContextType = CallbackContext[ExtBot, Dict, Dict, Dict] . This made sence for the type hinting in the builders, but it seemed clever to me to make this also publically available to allow users to improve their type hints. DefaultContextType is the correct annotation if you're not using a custom ContextTypes object. The import of DefaultContextType can be fine tuned in the context of Clear up imports policy #2468
  • Regarding type hints: At first I tried to make it so that already the type hints would indicate if e.g. builder.bot(bot) would fail because you already set a token. I quickly gave up on this because it became too excessive. Instead type hinting "only" covers job_queue, bot, persistence, context_types including bot/chat/user_data. This essentially makes sure that one can get proper type hints for all the attributes of the context argument. E.g. now it tells you if context.bot is tg.Bot or ExtBot and also if context.job_queue is None
  • EDIT - More notes on type hints: set_dispatcher/updater_class don't have proper type hints, i.e. you won't know if updater or updater/context.dispatcher are instances of subclasses. IISC that's near impossible to get right and users that are so advanced that they want custom subclasses, should be able to handle that …

TODO:

  • Add convenience methods Updare/Dispatcher.builder() -> Updater/DispatcherBuilder() and use those in the examples
  • Fix test_dispatcher/test_less_than_one_worker_warning
  • Update, if Rename private modules with leading underscore #2687 is merged first
  • Check if I can up the coverage a bit more

Checklist for PRs

  • Added .. versionadded:: version, .. versionchanged:: version or .. deprecated:: version to the docstrings for user facing changes (for methods/class descriptions, arguments and attributes)
  • Created new or adapted existing unit tests

Bibo-Joshi and others added 29 commits August 11, 2021 08:44
…ryptionError (#2621)

* move telegramdecryptionerror to error.py

* Change error class name
* 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>
Co-authored-by: Hinrich Mahler <22366557+Bibo-Joshi@users.noreply.github.com>
# Conflicts:
#	telegram/ext/conversationhandler.py
#	telegram/ext/dispatcher.py
#	telegram/ext/updater.py
#	tests/conftest.py
#	tests/test_dispatcher.py
#	tests/test_jobqueue.py
#	tests/test_persistence.py
#	tests/test_updater.py
# Conflicts:
#	telegram/ext/__init__.py
#	telegram/ext/dispatcher.py
#	telegram/utils/promise.py
# Conflicts:
#	telegram/ext/__init__.py
#	telegram/ext/callbackcontext.py
#	telegram/ext/conversationhandler.py
#	telegram/ext/dispatcher.py
#	telegram/ext/jobqueue.py
#	telegram/ext/updater.py
#	tests/conftest.py
#	tests/test_dispatcher.py
#	tests/test_jobqueue.py
#	tests/test_persistence.py
#	tests/test_updater.py
Bibo-Joshi and others added 4 commits September 17, 2021 17:48
Co-authored-by: Harshil <37377066+harshil21@users.noreply.github.com>
# Conflicts:
#	telegram/ext/jobqueue.py
Bibo-Joshi and others added 7 commits September 22, 2021 16:49
Co-authored-by: Harshil <37377066+harshil21@users.noreply.github.com>
# Conflicts:
#	examples/deeplinking.py
#	examples/inlinebot.py
#	telegram/ext/dispatcher.py
#	telegram/ext/updater.py
#	tests/conftest.py
#	tests/test_dispatcher.py
#	tests/test_updater.py
# Conflicts:
#	telegram/bot.py
#	telegram/ext/__init__.py
#	telegram/ext/callbackcontext.py
#	telegram/ext/conversationhandler.py
#	telegram/ext/dispatcher.py
#	telegram/ext/extbot.py
#	telegram/ext/jobqueue.py
#	telegram/ext/updater.py
#	tests/conftest.py
#	tests/test_bot.py
#	tests/test_dispatcher.py
#	tests/test_jobqueue.py
#	tests/test_persistence.py
#	tests/test_stack.py
#	tests/test_updater.py
# Conflicts:
#	examples/passportbot.py
#	examples/persistentconversationbot.py
Copy link
Member

@tsnoam tsnoam left a comment

Choose a reason for hiding this comment

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

Left 2 minor comments.
PR approved. Merge at will.

"""
return self._set_request(request)

def private_key(self: BuilderType, private_key: bytes, password: bytes = None) -> BuilderType:
Copy link
Member

Choose a reason for hiding this comment

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

I think that a really nice touch would be to have private_key accept either bytes (the key itself) or Path|str and then we'll open the file and read the file for the user.

Anyway, lets merge this PR already, and if you like the idea about the private_key, lets do it in another PR.

@Bibo-Joshi Bibo-Joshi merged commit 7ba5b3a into v14 Oct 9, 2021
@Bibo-Joshi Bibo-Joshi deleted the builder-pattern-for-ext branch October 9, 2021 11:56
@Bibo-Joshi Bibo-Joshi mentioned this pull request Oct 9, 2021
@github-actions github-actions bot locked and limited conversation to collaborators Oct 10, 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
🛠 breaking change type: breaking 🔌 enhancement pr description: enhancement 🛠 refactor change type: refactor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants