-
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
Sorry, something went wrong.
All reactions
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
Sorry, something went wrong.
All reactions
Current approach of Also fails when there are extra args and the constructor does not accept **kwargs. |
All reactions
Sorry, something went wrong.
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. |
All reactions
Sorry, something went wrong.
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...
Sorry, something went wrong.
All reactions
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.
Sorry, something went wrong.
All reactions
@Bibo-Joshi Nope, all done and am happy with it now |
All reactions
Sorry, something went wrong.
Bibo-Joshi
Successfully merging this pull request may close these issues.
None yet
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.