-
Notifications
You must be signed in to change notification settings - Fork 5.7k
Remove BP.insert/replace_bot
#2893
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
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.
Looks good overall IMO :) Was able to go right into nitpicking ;P
This was done since Bot's init doesn't accept **kwargs so custom deepcopying/pickle would fail.
Also some code cleanup
provides speed benefits for py 3.8+ users
Attributes won't be assigned to the new object otherwise
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.
Thanks for the work on this! I left a number of comments.
Additionally, please elaborate in the docstring of Bot
why bots should not be serialized and that for that reason pickling will raise an exception
Current approach of Also fails when there are extra args and the constructor does not accept **kwargs. |
Another way to achieve deepcopying without actually defining import copy
from telegram.ext._utils.stack import was_called_by
...
if was_called_by(inspect.currentframe(), Path(copy.__file__)):
return self._get_attrs(include_private=True, recursive=False)
return self._get_attrs(include_private=True, recursive=False, remove_bot=True) but didn't use this since it feels a bit hacky and unpythonic, although its elegant. |
return self._bot | ||
if pid == _REPLACED_UNKNOWN_BOT: | ||
return None | ||
raise pickle.UnpicklingError("Found unknown persistent id when unpickling!") |
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.
codecov gonna complain about this since I don't think it's possible to test this...
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.
Finally get's a LGTM from me 😃 Thanks for the effort you put into this! Is there anything left from your side? Otherwise we can merge.
@Bibo-Joshi Nope, all done and am happy with it now |
Closes #2882
Checklist for PRs
.. versionadded:: version
,.. versionchanged:: version
or.. deprecated:: version
to the docstrings for user facing changes (for methods/class descriptions, arguments and attributes)AUTHORS.rst
(optional)Breaking changes:
insert/replace_bot
was removed so user may have to useset_bot(bot)
before using shortcuts.