- Notifications
You must be signed in to change notification settings - Fork5.7k
Updater improvements#1018
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Updater improvements#1018
Uh oh!
There was an error while loading.Please reload this page.
Conversation
telegram/ext/updater.py Outdated
| while 1: | ||
| def _bootstrap(self, max_retries, clean, webhook_url, allowed_updates, cert=None, | ||
| bootstrap_interval=5): | ||
| retries = [0] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Why did you make this a list? It's still only used for one int value
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Because otherwise it will overridden as a local variable of the inner function.
A different workaround would be to do:
globalretriesretries=0
But I don't likeglobal inside such an inner code.
Of course we can do it a class method or something more "generic" but then we'll lose code locality.
If you can think of something else, I'll be happy for a more elegant solution.
telegram/ext/updater.py Outdated
| return False | ||
| def bootstrap_clean_updates(): | ||
| self._clean_updates() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The_clean_updates() method is only ever called once here. I suggest the code is added in this place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
mmm... I tried not to touch_clean_updates() (move less code if possible)
but I guess you're right, code locality is preferred here.
telegram/ext/updater.py Outdated
| sleep(1) | ||
| self._network_loop_retry(bootstrap_set_webhook, bootstrap_onerr_cb, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
If I read it right, this means a webhook is set whenUpdater.start_polling is called, but with an empty url. This is an unnecessary API call to a method that is prone to flood actions. I would like to prevent it.
Also see the comment on delete webhook
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This call was "written in blood".
If user had messed around with webhooks and now he wants a polling server, we make sure that Telegram servers are properly configured for it.
@jh0ker Maybe you remember the origin of this better?
telegram/ext/updater.py Outdated
| else: | ||
| self.logger.exception(msg) | ||
| raise | ||
| def bootstrap_del_webhook(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This is also called whenUpdater.start_webhook() is called, but this is not necessary. However the set/delete_webhook methods are prone to flood limits. I would like to prevent that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Same as above.
@jh0ker ?
| raise | ||
| self.logger.debug('{0} - ended'.format(thr_name)) | ||
| def start_polling(self, | ||
| poll_interval=0.0, | ||
| timeout=10, | ||
| clean=False, | ||
| bootstrap_retries=0, | ||
| bootstrap_retries=-1, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The docstring doesn;t reflect this change to the default value
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
will fix
I just left some review comments. |
…trap_retries value[ci skip]
Yes good! |
| * < 0 - retry indefinitely (default) | ||
| * 0 - no retries |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
documentation was updated to state indefinite retries, but the default value in the signature was not updated
Uh oh!
There was an error while loading.Please reload this page.
fixes#605